Copilot commented on code in PR #3039:
URL: https://github.com/apache/hugegraph/pull/3039#discussion_r3328521469
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -171,6 +172,10 @@ public static void extractHasContainer(HugeGraphStep<?, ?>
newStep,
step = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
+ if (followedByMatchStep(step) &&
+ hasIndexRequiredPredicate(holder)) {
+ break;
+ }
Review Comment:
This guard disables index-based query pushdown for `has(...)` filters before
a `match()` step whenever a `neq`/range predicate is present, regardless of
whether the property is actually indexed. For users whose schema does provide a
matching secondary/range index (e.g. the tests at `VertexCoreTest.java:6473`
and `EdgeCoreTest.java:7144` that index a boolean property and then use
`match(__.as(...).where(has(prop, P.lt(true))))`), this means the leading
`has(prop, P.neq/lt/gt(...))` filter will no longer be pushed into the backend
`ConditionQuery` and will instead fall back to an in-memory TinkerPop filter
scan, which can be a substantial performance regression for selective
predicates over large vertex/edge sets.
Consider scoping the guard more narrowly so we only skip extraction when the
property is genuinely not indexed (e.g. by consulting the schema/index labels
for the involved vertex/edge labels, similar to the matching done in
`GraphIndexTransaction.matchSingleOrCompositeIndex`/`matchRangeOrSearchIndexLabels`).
At minimum, this tradeoff is worth calling out explicitly in a comment on
`extractHasContainer` so future maintainers understand why
otherwise-extractable filters are being held back.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -171,6 +172,10 @@ public static void extractHasContainer(HugeGraphStep<?, ?>
newStep,
step = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
+ if (followedByMatchStep(step) &&
+ hasIndexRequiredPredicate(holder)) {
+ break;
+ }
Review Comment:
The guard is applied per-`HasStep`, and uses `break` to exit the extraction
loop entirely. As a result, if a traversal is shaped like `V().has("a",
P.neq(x)).has("b", "value").match(...)`, not only is the `has("a", neq)` filter
held back, but the subsequent safe `has("b", "value")` (equality, always
index-compatible / pushdown-safe) is also left in the traversal and not
extracted into the `HugeGraphStep`. Consider replacing `break` with `continue`
(and only skipping the current holder) so that adjacent extractable `HasStep`s
before the `match()` can still be pushed into the backend query.
--
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]
