bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3939865403
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +659,162 @@ private static boolean
extractHasContainers(HugeVertexStep<?> newStep,
private static boolean canExtractHasContainers(HugeGraph graph,
HasContainerHolder holder) {
- for (HasContainer has : holder.getHasContainers()) {
+ // Keep unsafe labels and their sibling properties for local filtering.
+ if (hasUnsafeLabelPredicate(holder)) {
+ return false;
+ }
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ for (HasContainer has : hasContainers) {
if (!canExtractHasContainer(graph, has)) {
return false;
}
}
return true;
}
+ private static void prepareLocalHasContainers(
+ Step<?, ?> source, Traversal.Admin<?, ?> traversal) {
+ QueryHolder query = (QueryHolder) source;
+ Step<?, ?> step = source.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> next = step.getNextStep();
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : new
ArrayList<>(holder.getHasContainers())) {
+ // Paging is query metadata, never an element property
filter.
+ if (QueryHolder.SYSPROP_PAGE.equals(has.getKey())) {
+ query.addHasContainer(has);
+ holder.removeHasContainer(has);
+ continue;
+ }
+ if (isSysProp(has.getKey())) {
+ continue;
+ }
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates,
ImmutableList.of(has.getPredicate()));
+ if (predicates.stream().anyMatch(p ->
+ p.getBiPredicate() ==
Condition.RelationType.TEXT_CONTAINS)) {
+ HugeGraph graph = getGraph(source);
Review Comment:
⚠️ **`getGraph()` throws where this path previously tolerated a null graph**
Evidence: `getGraph()` (TraversalUtil.java:108-114) throws
`IllegalArgumentException("There is no graph in step: ...")` as soon as
`tryGetGraph()` returns null. The code this branch replaces did the opposite:
`canExtractHasContainer()` returns `false` on `graph == null`
(TraversalUtil.java:823-825), so an unresolvable graph degraded to local
filtering instead of failing. Both `HugeVertexStepStrategy.apply()` and
`convAllHasSteps()` (TraversalUtil.java:1318-1334) document that state as
reachable in child traversals.
Impact: in any child traversal whose graph is not resolvable, a source step
whose chain carries both a `Text.contains()` predicate and a downstream non-EQ
label predicate now aborts the whole query here, before the null check that the
old path had.
Requested change: use `tryGetGraph(source)` and skip the SEARCH rewrite when
it returns null. The untouched predicate still evaluates locally as a substring
match (`Condition.java:94-97`), so the fallback is degraded matching rather
than a failed traversal.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +659,162 @@ private static boolean
extractHasContainers(HugeVertexStep<?> newStep,
private static boolean canExtractHasContainers(HugeGraph graph,
HasContainerHolder holder) {
- for (HasContainer has : holder.getHasContainers()) {
+ // Keep unsafe labels and their sibling properties for local filtering.
+ if (hasUnsafeLabelPredicate(holder)) {
+ return false;
+ }
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ for (HasContainer has : hasContainers) {
if (!canExtractHasContainer(graph, has)) {
return false;
}
}
return true;
}
+ private static void prepareLocalHasContainers(
+ Step<?, ?> source, Traversal.Admin<?, ?> traversal) {
+ QueryHolder query = (QueryHolder) source;
+ Step<?, ?> step = source.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> next = step.getNextStep();
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : new
ArrayList<>(holder.getHasContainers())) {
+ // Paging is query metadata, never an element property
filter.
+ if (QueryHolder.SYSPROP_PAGE.equals(has.getKey())) {
+ query.addHasContainer(has);
+ holder.removeHasContainer(has);
+ continue;
+ }
+ if (isSysProp(has.getKey())) {
+ continue;
+ }
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates,
ImmutableList.of(has.getPredicate()));
+ if (predicates.stream().anyMatch(p ->
+ p.getBiPredicate() ==
Condition.RelationType.TEXT_CONTAINS)) {
+ HugeGraph graph = getGraph(source);
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new HasContainer(has.getKey(),
+ localSearchPredicate(has.getPredicate(),
graph)));
+ }
+ }
+ if (holder.getHasContainers().isEmpty()) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ }
+ step = next;
+ }
+ if (query.queryInfo().paging() && step instanceof RangeGlobalStep) {
+ // Bound the raw backend page even when local filters prevent
normal
+ // range extraction. Keep the range step to apply offset/limit
after
+ // those filters; a filtered page may contain fewer results.
+ query.setRange(0, ((RangeGlobalStep<?>) step).getHighRange());
+ }
+ }
+
+ private static P<?> localSearchPredicate(P<?> predicate, HugeGraph graph) {
+ if (predicate instanceof ConnectiveP) {
+ List<P<Object>> children = new ArrayList<>();
+ for (P<?> child : ((ConnectiveP<?>) predicate).getPredicates()) {
+ @SuppressWarnings("unchecked")
+ P<Object> converted = (P<Object>) localSearchPredicate(child,
graph);
+ children.add(converted);
+ }
+ return predicate instanceof AndP ? new AndP<>(children) : new
OrP<>(children);
+ }
+ if (predicate.getBiPredicate() !=
Condition.RelationType.TEXT_CONTAINS) {
+ return predicate.clone();
+ }
+ // Match SEARCH terms in place, preserving range and side-effect
ordering.
+ Predicate<Object> matcher = graph.searchPredicate((String)
predicate.getValue());
+ return new P<>((actual, ignored) -> matcher.test(actual),
predicate.getValue());
+ }
+
+ private static boolean hasUnsafeLabelInTraversal(
+ Traversal.Admin<?, ?> traversal, Step<?, ?> sourceStep) {
+ // Partial pushdown can lose candidates before local label filtering.
+ // Scan conservatively across the remaining traversal and its children;
+ // arbitrary extension steps don't reliably expose element identity.
+ // FIXME: Restore selective pushdown when every candidate schema label
+ // has compatible index coverage for extracted property predicates.
+ List<Step> steps = traversal.getSteps();
+ int start = 0;
+ while (start < steps.size() && steps.get(start) != sourceStep) {
+ start++;
+ }
+ start++;
+ for (int i = start; i < steps.size(); i++) {
+ Step<?, ?> step = steps.get(i);
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ if (hasUnsafeLabelPredicate(holder)) {
+ return true;
+ }
+ }
+ if (hasUnsafeLabelInChildren(step)) {
Review Comment:
⚠️ **The unsafe-label gate is one-directional, so the mirror of the case it
protects still loses results**
Evidence: `hasUnsafeLabelInTraversal()` (lines 736-762) walks forward from
`sourceStep` within `traversal` and recurses downward into each step's local
and global children. It never inspects `traversal.getParent()`. So the gate
fires when the source step is in the parent and the non-EQ label predicate is
in a child
(`TraversalUtilOptimizeTest#testExtractHasContainerKeepsGlobalChildUnsafeLabelLocal`),
but not for the mirror image: a source step inside a child traversal whose
property predicate is pushed down, with the non-EQ label predicate sitting in
the parent and applied to exactly what that child produced.
Impact: in that shape the property predicate still reaches a per-label index
query, so elements whose schema label has no matching index are dropped before
the negative label filter ever runs. That is the same loss this PR set out to
prevent. It is pre-existing rather than introduced here, which is why I am not
marking it blocking.
Requested change: either extend the check through `traversal.getParent()`
when the source step sits in a child traversal, or record in the FIXME above
that the gate is deliberately one-directional, so the remaining gap is
documented rather than implied to be covered.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -480,14 +485,18 @@ private IdHolderList queryByUserprop(ConditionQuery
query) {
}
}
}
+ boolean paging = query.paging();
+ if (query.containsConditionValues(HugeKeys.LABEL) &&
Review Comment:
🧹 **LABEL is resolved repeatedly on one index query**
Evidence: `conditionValues()` walks every condition and allocates two lists
plus an intersection set per call. On the userprop path it now runs three
times, at line 404 (`singleConditionValueOrNull`), lines 489-490, and line 781,
plus two cheaper `containsConditionValues()` scans at 489 and 780. On the label
path it runs twice, at lines 404 and 422.
Requested change (fine as a follow-up rather than in this PR): resolve the
label set once in `queryIndex()` and hand the resolved `Set<Object>` or `Id` to
`queryByLabel()`, `queryByUserprop()` and `collectMatchedIndexes()`.
--
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]
