bitflicker64 commented on code in PR #3182:
URL: https://github.com/apache/hugegraph/pull/3182#discussion_r3906902364
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1870,10 +1872,10 @@ private void removeLeftIndexIfNeeded(Map<Id,
HugeVertex> vertices) {
}
}
- private <T extends HugeElement> Iterator<T> filterUnmatchedRecords(
+ private <T extends HugeElement> Iterator<T> filterInvalidRecords(
Review 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"`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java:
##########
@@ -441,6 +449,29 @@ protected Iterator<HugeEdge> queryEdgesFromBackend(Query
query) {
return new ExtendableIterator<>(edges.iterator(), rs);
}
+ private static boolean queryNeedsPostFilter(Query query) {
Review 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", ...)`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -861,6 +861,7 @@ protected Iterator<HugeVertex>
queryVerticesFromBackend(Query query) {
Iterator<HugeVertex> vertices = new MapperIterator<>(entries,
this::parseEntry);
vertices = this.filterExpiredResultFromBackend(query, vertices);
+ vertices = this.filterUnmatchedRecords(vertices, query);
Review 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.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java:
##########
@@ -420,6 +424,10 @@ protected Iterator<HugeEdge> queryEdgesFromBackend(Query
query) {
}
Iterator<HugeEdge> rs = super.queryEdgesFromBackend(query);
+ if (queryNeedsPostFilter(query)) {
Review 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`.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -845,7 +845,7 @@ public Iterator<Vertex> queryVertices(Query query) {
query.resetActualOffset();
Iterator<HugeVertex> results = this.queryVerticesFromBackend(query);
- results = this.filterUnmatchedRecords(results, query);
+ results = this.filterInvalidRecords(results, query);
Review Comment:
⚠️ The split also reverses the two predicates relative to each other, which
looks incidental rather than intended.
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`,
so `this.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 `filterUnmatchedRecords` already 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.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/VertexCoreTest.java:
##########
@@ -5424,7 +5443,7 @@ public void
testQueryByJointIndexesWithSearchAndTwoRangeIndexesAndWithin() {
graph.addVertex(T.label, "test", "name", "诚信文明",
"confirmType", 3, "type", 1, "kid", 3);
- this.mayCommitTx();
+ this.commitTx();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
