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% 36.98% -0.81%
+ Complexity 6556 6348 -208
============================================
Files 800 785 -15
Lines 68929 66845 -2084
Branches 9157 8892 -265
============================================
- Hits 26046 24720 -1326
+ Misses 39824 39189 -635
+ Partials 3059 2936 -123 ☔ 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.
2 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