fix(server): filter index results before input ordering - #3182
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3182 +/- ##
============================================
- Coverage 37.78% 37.77% -0.02%
- Complexity 6556 6559 +3
============================================
Files 800 800
Lines 68929 68960 +31
Branches 9157 9166 +9
============================================
+ Hits 26046 26050 +4
- Misses 39824 39843 +19
- Partials 3059 3067 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
contrueCT
changed the title
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The reordering is sound: moving index-candidate matching upstream of keepInputOrderIfNeeded means each record is tested while the shared resultsFilter still belongs to its own flattened sub-query, and the new cache guard compensates for filterUnmatchedRecords no longer running in queryVertices/queryEdges. Below are one hardening item, one question about a behavior change that looks incidental, and three maintenance items. Evidence: reviewed at head via git diff $(git merge-base FETCH_HEAD origin/master) FETCH_HEAD; queryNeedsPostFilter checked against every branch of rightResultFromIndexQuery (GraphTransaction.java:1915-1979); CI green at head apart from the codecov/project coverage threshold.
One finding has no line to anchor to, so it goes here. .github/images/issue-3180-query-filter-order.png adds 1,033,655 bytes to the repository. origin/master has no image files anywhere in the tree and no .github/images/ directory, so this introduces both a new directory convention and the repository's first binary asset. The PR body already loads the diagram from raw.githubusercontent.com on a fork SHA, so the description renders whether or not the file lands in apache/hugegraph. Please upload it through GitHub's own attachment upload on the PR description and drop the file from the diff.
| } | ||
|
|
||
| private <T extends HugeElement> Iterator<T> filterUnmatchedRecords( | ||
| private <T extends HugeElement> Iterator<T> filterInvalidRecords( |
There was a problem hiding this comment.
HugeFactoryAuthProxy.java:273 registers the method being renamed here, "filterUnmatchedRecords", with Reflection.registerMethodsToFilter. The new sibling filterInvalidRecords is not registered, so it remains enumerable via getDeclaredMethods() while the method it was split out of is hidden. This affects enumeration only: filterInvalidRecords is still private and access control is unchanged.
Please add "filterInvalidRecords" to the Reflection.registerMethodsToFilter(GraphTransaction.class, ...) list next to "filterUnmatchedRecords".
| return new ExtendableIterator<>(edges.iterator(), rs); | ||
| } | ||
|
|
||
| private static boolean queryNeedsPostFilter(Query query) { |
There was a problem hiding this comment.
🧹 This predicate has to track GraphTransaction.rightResultFromIndexQuery (GraphTransaction.java:1915-1979), which decides whether a record is actually dropped, but nothing links the two.
The pairing is safe today. rightResultFromIndexQuery returns true early for a non-ConditionQuery whose immediate origin is also not a ConditionQuery (1920-1925), for edge + LABEL + conditions().size() == 1, and for edge + LABEL + optimized() == INDEX; otherwise it falls through to optimized() == NONE || cq.test(elem) at 1948. queryNeedsPostFilter allows caching only for the INDEX and NONE shapes, so it is strictly more conservative. The risk is drift: the rule lives in two classes in two shapes, and editing one would silently let the caches serve unmatched records.
Please derive both from a single helper. Note the refactor also has to move CachedGraphTransactionTest.testQueryNeedsPostFilter, which calls Whitebox.invokeStatic(CachedGraphTransaction.class, ..., "queryNeedsPostFilter", ...).
| Iterator<HugeVertex> vertices = new MapperIterator<>(entries, | ||
| this::parseEntry); | ||
| vertices = this.filterExpiredResultFromBackend(query, vertices); | ||
| vertices = this.filterUnmatchedRecords(vertices, query); |
There was a problem hiding this comment.
🧹 On the non-paging edge branch the filter runs in queryEdgesFromBackendInternal(cq) with cq already flattened, and re-flattening yields exactly one sub-query (ConditionQueryFlatten.flattenRelations returns ImmutableList.of(cq)), so cq owns its resultsFilter. Paging and non-ConditionQuery edge queries skip that flatten and go straight to queryEdgesFromBackendInternal at line 1080.
The vertex path has no outer flatten at all: this.query(query) flattens internally and every sub-query pushes its filter onto this same shared query through ConditionQuery.updateResultsFilter (ConditionQuery.java:708-727), called from QueryList.java:223.
It is correct today only because FilterIterator.fetch() tests each element immediately and WrappedIterator.hasNext() returns the buffered element, so the advance into the next sub-query always follows the test. Please state that invariant here, or bind the filter to the sub-query the records came from.
| } | ||
|
|
||
| Iterator<HugeEdge> rs = super.queryEdgesFromBackend(query); | ||
| if (queryNeedsPostFilter(query)) { |
There was a problem hiding this comment.
🧹 This reads as dead code: the same call ran at line 402 on the same object, and the comment does not say why the answer can change.
It is not dead. ConditionQuery.optimized() propagates up the originQuery chain (ConditionQuery.java:681-696) and ConditionQuery.copy() sets originQuery(this) (line 577), so the flattened children point back at this query. super.queryEdgesFromBackend(query) reaches GraphIndexTransaction.queryIndex (line 383), which sets INDEX at line 402, and INDEX_FILTER can be set at line 609. So query.optimized() can move off NONE between the two checks.
Please expand the comment to name that, for example: super.queryEdgesFromBackend() may promote query.optimized() via origin-chain propagation, so re-check before caching.
| "confirmType", 3, "type", 1, "kid", 3); | ||
|
|
||
| this.mayCommitTx(); | ||
| this.commitTx(); |
There was a problem hiding this comment.
🧹 Making the regression deterministic is the right call, since the bug only reproduces once the rows are in the backend store. But mayCommitTx() is a coin flip (BaseCoreTest.java:141-146), so this test used to exercise the uncommitted-transaction path roughly half the time, and that coverage is now gone rather than merely made reliable.
The uncommitted path is not incidental to this change: ConditionQuery.test deliberately skips resultsFilter for fresh elements (ConditionQuery.java:625), so it is a different branch of the code this PR touches.
Please keep the deterministic committed assertions and restore the other half, either by repeating the three query blocks after a mayCommitTx() or by adding a sibling test that queries before committing.
|
|
||
| Iterator<HugeVertex> results = this.queryVerticesFromBackend(query); | ||
| results = this.filterUnmatchedRecords(results, query); | ||
| results = this.filterInvalidRecords(results, query); |
There was a problem hiding this comment.
Before this PR a single filter ran the undefined-label warning, then hidden, then deleting-label, then rightResultFromIndexQuery. Now filterUnmatchedRecords runs upstream (864 and 1104) and filterInvalidRecords runs downstream (here and 1015). Two consequences:
- Hidden and deleting-label elements now reach
rightResultFromIndexQuery, sothis.indexTx.asyncRemoveIndexLeft(cq, elem)(1939, 1955, 1972) can be scheduled for elements whose schema label is already being deleted. Previously they were dropped before reaching that point. - The "Left record is found" warning (1882-1884) is now unreachable on index queries for any record
filterUnmatchedRecordsalready dropped, which is the left-index case that warning exists for.
Please confirm the reordering is intended, and if it is, either restore the warning for left records on index queries or note why it is no longer needed.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new valid-record wrapper hides internal task vertices from task lookup, so schema/index operations cannot wait for their asynchronous tasks. Evidence: at exact head 88ba98c, the local EdgeCoreTest#testQueryByTextContainsProperty fails at the first index-label create with NotFoundException: Can't find task with id '1', while the base commit passes; the latest server and hstore CI failures show the same missing-task pattern.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new queryValid*FromBackend() wrappers apply showHidden and showDeleting filtering to Query objects the transaction builds for itself, which never carried those flags, so the regression already reported for task lookup also lands on auth relationship lookup by edge id and on adjacent-edge collection during vertex deletion. Evidence: CI at this head fails on memory, rocksdb, hbase, hstore and both macOS rocksdb jobs; build-server (rocksdb, 11) reports 814 tests, 29 failures and 619 errors, dominated by 573 occurrences of Failed to wait for task 'N' completed with Caused by: java.lang.AssertionError: N from StandardTaskScheduler.java:532, while master at the merge base 98477f0 is green. The task-lookup call site at line 808 is already covered by an existing review and is not repeated below.
|
This PR is intended as a correctness hotfix for #3180 to stabilize the current master CI. A follow-up issue will track explicit query-batch boundaries in QueryResults / InputOrderIterator and remove the remaining dependency on cross-query prefetch and mutable shared query state. |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The reordering holds up at this head and the regressions reported against 88ba98c are gone: filterUnmatchedRecords now runs upstream of keepInputOrderIfNeeded, so each record is tested while resultsFilter still belongs to its own flattened sub-query, the queryValid*FromBackend() wrappers are off every internal call site, and queryNeedsPostFilter is never less conservative than rightResultFromIndexQuery on any chain shape I could construct. Two documentation-grade items below.
Evidence:
git diff 98477f0 acbee16andgit diff 88ba98c acbee16read in full.queryNeedsPostFilter(GraphTransaction.java:2012-2033) traced against every branch ofrightResultFromIndexQuery(1946-2010).OptimizedTypeordinals (ConditionQuery.java:805-811) plus the max-keeping propagation inConditionQuery.optimized(...)(681-696) mean a mixed flatten promotes the parent past theINDEXcarve-out, so the re-check at CachedGraphTransaction.java:424 declines to cache.InputOrderIterator.fetchBatch(QueryResults.java:378-419) tolerates ids filtered out upstream, so moving the filter above it does not break order restoration.- The ramtable short-circuit at CachedGraphTransaction.java:391-394 now returns before
filterUnmatchedRecords, which on master ran downstream inqueryEdges(Query). Benign, because that branch runs beforethis.query(...), so the caller'soptimized()is stillNONEand the old filter was a no-op on those rows; hidden and deleting labels are still handled byfilterInvalidRecords. Worth knowing it changed, though. HugeEdge.sysprop(321-347) returnsfatherId()forLABELand the own id forSUB_LABEL, so filtering pertempQueryon theparentElQueryWithSortKeyspath tests correctly.gh pr checks 3182is green on 23 of 24 checks at this head, the exception being thecodecov/projectthreshold.
Findings already raised on this PR are not repeated.
| query.resultType().isVertex() != elem.type().isVertex() || | ||
| rightResultFromIndexQuery(query, elem); | ||
| if (!matched) { | ||
| warnLeftRecord(elem); |
There was a problem hiding this comment.
🧹 The LOG.warn behind this call cannot fire for a query that leaves showHidden at its default, which is narrower than the block above reads.
A left record's label name is SchemaElement.UNDEF, "~undefined" (SchemaElement.java:57), returned by HugeVertex.label() (198-200) and HugeEdge.label() (117-119), and Graph.Hidden.isHidden is a "~" prefix test. So invalidRecord at 1912 is true for every left record when query.showHidden() is false, and the early return true runs before control reaches 1920.
The warning is not lost. On queryVertices(Query) and queryEdges(Query) it comes from filterInvalidRecords at 1890, which is where master emitted it. Line 1921 is reached only by the showHidden(true) callers, such as EntityManager.queryEntity (158) and traverseByLabel (2336). On the internal call sites this PR routes through filterUnmatchedRecords with no downstream filterInvalidRecords (380, 806, 983, 990, 1809, 1874) it adds nothing, since all six build their own query and leave showHidden false (Query.java:99).
Requested change: record that split in the comment at 1907-1911, naming 1890 as the call that covers ordinary queries. As written the block reads as though this line restores left-record logging for index queries in general.
| if (this.enableCacheVertex() && | ||
| query.idsSize() > 0 && query.conditionsSize() == 0) { | ||
| query.idsSize() > 0 && query.conditionsSize() == 0 && | ||
| !queryNeedsPostFilter(query)) { |
There was a problem hiding this comment.
🧹 This guard decides before the backend call. The edge path re-checks after super.queryEdgesFromBackend(query) at line 424, and its new comment gives the reason: that call can promote query.optimized() through origin-query propagation. Nothing records why the vertex path needs no equivalent.
It does not need one, and the reason is worth stating, because the clause is live rather than defensive. queryNumber falls back to this.queryVertices(q) at GraphTransaction.java:597-598 with q being whatever optimizeQueries handed its fetcher (561), so q arrives here as an IdQuery whose origin ConditionQuery already carries PRIMARY_KEY, set at 1547 before the IdQuery is built at 1556, or INDEX, set at GraphIndexTransaction.java:402 while indexQuery runs. The PR's own testQueryByPrimaryValuesAndPropsWithCachedVertex covers the first shape. optimized() is therefore already fixed when this line runs, and queryVerticesByIds cannot promote it further: GraphTransaction.query(Query) (535-540) short-circuits to super.query(query) for a non-ConditionQuery, so optimizeQueries never runs beneath it.
Requested change: add a one-line comment here mirroring the one at 424. The asymmetry currently reads as an oversight.
Brings the nine commits helm-dev was behind: apache#3140, apache#3149, apache#3159, apache#3171, apache#3173, apache#3176, apache#3177, apache#3178 and apache#3182. The three conflicts were early copies of apache#3159 and apache#3171 already carried on this branch (docker-build-ci.yml, Dockerfile-hstore, ApiVersion.java) and resolve to the master side, so the tree now differs from master only in the chart, its CI workflow, the README pointer to it and the tgz excludes.
Picks up apache#3182 (index result filtering) and the three fixes this chart depends on: apache#3185 (quorum-aware PD /v1/ready and raft gauges), apache#3187 (configurable Server startup timeout) and apache#3189 (PD REST credential validation, 401 on refusal). No conflicts; the branch touches only helm/, the chart CI workflow and the README chart section.
3 participants
Purpose of the PR
within()conditions with search or otherpost-filtered indexes and can drop valid vertices or edges.
Root cause
HStore does not sort these backend results by input IDs. HugeGraph therefore restores input
order with
InputOrderIterator, which may prefetch the next flattened subquery. That prefetchupdates the shared origin query's
resultsFilter. Previously, record matching happened afterinput-order restoration, so rows from subquery A could be checked with subquery B's filter.
CI follow-up
Count-query fallbacks wrap the original
ConditionQueryin anIdQuery. A transaction-cachehit could therefore return index candidates before the original condition was applied. The
cache eligibility check now follows the origin-query chain, and both vertex and edge caches are
bypassed whenever any query in that chain still needs post-filtering.
Main Changes
boundary.
post-filtering, including search indexes,
INDEX_FILTER,SORT_KEYS, andINDEXquerieswithout a label.
equivalent edge regression plus cache-classification coverage.
Verifying these changes
mvn test -pl hugegraph-server/hugegraph-test -am -P unit-testcore-test,hstoreregression setcore-test,memoryregression setcore-test,rocksdbregression setmvn clean compile -Dmaven.javadoc.skip=truemvn editorconfig:formatDoes this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need