bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3943320315
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1957,7 +1957,8 @@ private boolean rightResultFromIndexQuery(Query query,
HugeElement elem) {
}
ConditionQuery cq = (ConditionQuery) query;
- if (cq.condition(HugeKeys.LABEL) != null && cq.resultType().isEdge()) {
+ if (cq.singleConditionValueOrNull(HugeKeys.LABEL) != null &&
Review Comment:
🧹 **This is migrated, but its sibling gate below is the one production
`condition(HugeKeys.LABEL)` left**
Evidence: `queryNeedsPostFilter()` at line 2024 still reads
```java
boolean edgeIndexWithLabel =
cq.resultType().isEdge() &&
cq.optimized() == OptimizedType.INDEX &&
cq.condition(HugeKeys.LABEL) != null;
```
It is byte-identical at the merge base and outside every hunk of this PR,
but it now disagrees with the branch you migrated here. For a sole `LABEL IN
[a, b]`, `condition()` returns the raw list (`ConditionQuery.java:293-297`), so
`edgeIndexWithLabel` is true and the query is reported as needing no
post-filter, while this branch correctly falls through to `cq.test(elem)`.
`condition()` can also throw `IllegalStateException`
(`ConditionQuery.java:305`) from inside that caching decision where
`singleConditionValueOrNull()` returns null.
No wrong data is served today: the production consumer is
`CachedGraphTransaction` (lines 322, 399, 424), and what it caches was already
filtered by `super.queryEdgesFromBackend()`. This is about the two gates
reading the same way and about the exception surface.
Requested change: as a follow-up, since line 2024 is outside this diff,
migrate it to `singleConditionValueOrNull(HugeKeys.LABEL) != null`, or leave a
comment there recording that the legacy semantics are deliberate.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +663,278 @@ 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 (T.id.getAccessor().equals(has.getKey())) {
Review Comment:
‼️ **`~id` never reaches the source step, so a point lookup becomes an
unbounded full scan**
Evidence: on this path no has container reaches `newStep.hasContainers`
(only `~page` at line 690 and the paging range at line 734 are pushed).
`HugeGraphStep.makeQuery()` then takes the `this.hasContainers.isEmpty()`
branch and builds `new Query(type)`, and `vertices()` takes the `!hasIds()`
branch. At the merge base the same container went through
`GraphStep.processHasContainerIds()`, which set `graphStep.ids` and gave
`graph.vertices(this.ids)`.
So `g.V().hasId(one).limit(10).hasLabel(P.neq("other"))`, the shape asserted
at `VertexCoreTest.java:9268`, scans the whole vertex table. The surviving
`HasStep` also blocks `extractRange()`, so `limit(10)` is not pushed either,
and since exactly one vertex can match, the `RangeGlobalStep` drains the scan
to exhaustion. `Query` starts at `DEFAULT_CAPACITY = 800000` (`Query.java:51,
97`) and `BackendEntryIterator.checkCapacity()` throws `LimitExceedException`
past it, so above that size this query stops returning the vertex at all. The
neighbouring `g.V().hasId(one).toList()` at line 9266 keeps its point lookup,
so the test is green either way.
The gate's rationale does not apply to `~id`: an id fetch is label-agnostic
and complete, so keeping it local buys nothing.
Requested change: for `HugeGraphStep` sources, still call
`GraphStep.processHasContainerIds(newStep, has)` on `T.id` containers and fall
back to this local rewrite only when it returns false. Please assert the
resulting step ids in a regression, so the plan is pinned and not just the
result set.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +663,278 @@ 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 (T.id.getAccessor().equals(has.getKey())) {
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new HasContainer(has.getKey(),
+ localIdPredicate(has.getPredicate())));
+ continue;
+ }
+ if (T.label.getAccessor().equals(has.getKey())) {
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new
LocalLabelHasContainer(has.getPredicate()));
+ 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 = tryGetGraph(source);
+ if (graph == null) {
+ // Child traversals may not have a graph yet. Keep
+ // their original local predicate in that case.
+ continue;
+ }
+ 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<?> localIdPredicate(P<?> predicate) {
+ // Keep IDs local to preserve source-ID intersections and step
ordering.
+ // UUID/Element values need the same representation as
HugeElement.id(). Leave
+ // strings unchanged so HasContainer retains its string-ID comparison.
+ P<?> copy = predicate.clone();
+ List<P<Object>> leaves = new ArrayList<>();
+ collectPredicates(leaves, ImmutableList.of(copy));
+ for (P<Object> leaf : leaves) {
+ Object value = leaf.getValue();
+ if (value instanceof Collection) {
+ List<Object> values = new ArrayList<>();
+ for (Object item : (Collection<?>) value) {
+ values.add(localIdValue(item));
+ }
+ leaf.setValue(values);
+ } else {
+ leaf.setValue(localIdValue(value));
+ }
+ }
+ return copy;
+ }
+
+ private static Object localIdValue(Object value) {
+ if (value instanceof UUID || value instanceof Element) {
+ return HugeElement.getIdValue(HugeType.VERTEX, value);
+ }
+ return value;
+ }
+
+ private static final class LocalLabelHasContainer extends HasContainer {
+
+ private static final long serialVersionUID = 1L;
+
+ private LocalLabelHasContainer(P<?> predicate) {
+ super(T.label.getAccessor(), predicate.clone());
+ }
+
+ @Override
+ protected boolean testLabel(Element element) {
+ // Resolve against the actual element, not a graph captured while
+ // strategies run. Child traversals may be unbound or later cloned.
+ return testLabelPredicate(this.getPredicate(), ((HugeElement)
element).schemaLabel());
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ private static boolean testLabelPredicate(P<?> predicate, SchemaLabel
label) {
+ if (predicate instanceof ConnectiveP) {
+ boolean and = predicate instanceof AndP;
+ for (P<?> child : ((ConnectiveP<?>) predicate).getPredicates()) {
+ if (testLabelPredicate(child, label) != and) {
+ return !and;
+ }
+ }
+ return and;
+ }
+ BiPredicate<?, ?> bp = predicate.getBiPredicate();
+ Object value = predicate.getValue();
+ if (bp == Contains.within || bp == Contains.without) {
+ for (Object item : (Collection<?>) value) {
+ if (testLabelValue(Compare.eq, label, item)) {
+ return bp == Contains.within;
+ }
+ }
+ return bp == Contains.without;
+ }
+ return testLabelValue((BiPredicate<Object, Object>) bp, label, value);
+ }
+
+ private static boolean testLabelValue(BiPredicate<Object, Object>
predicate,
+ SchemaLabel label, Object value) {
+ if (value instanceof Number) {
+ value = IdGenerator.of(((Number) value).longValue());
+ }
+ return predicate.test(value instanceof Id ? label.id() : label.name(),
value);
+ }
+
+ 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 the remaining traversal, its children and each ancestor's
+ // remaining steps for filters on child output. Arbitrary extension
+ // steps don't reliably expose element identity, so stay conservative.
+ // FIXME: Restore selective pushdown when every candidate schema label
+ // has compatible index coverage for extracted property predicates.
+ List<Step> steps = traversal.getSteps();
Review Comment:
⚠️ **The gate fires for label predicates that can never filter this step's
output**
This is the cost side of what I asked for on 2026-09-02 and 2026-09-05, not
a request to undo it. The traversal-level condition and the `getParent()` walk
are the right shape; the ask here is to bound their breadth.
Evidence: the walk enumerates every remaining step of the traversal, plus
each ancestor's remaining steps and all their children, and
`hasUnsafeLabelPredicate()` (line 916) inspects only the key and the predicate.
Nothing tracks which elements a step filters. For
```groovy
g.V().has("city", "Beijing").out().has(T.label, P.neq("author"))
```
the label predicate applies to the `out()` results and never to the `g.V()`
candidates, yet the gate fires, `prepareLocalHasContainers()` leaves `city`
local, and `HugeGraphStep.makeQuery()` builds `new Query(VERTEX)`. An indexed
lookup becomes a full vertex scan, with the same `DEFAULT_CAPACITY` ceiling
noted above. The `.out().where(__.not(__.hasLabel("author")))` variant behaves
the same way through `hasUnsafeLabelInChildren()` line 884.
Requested change: bound the forward scan to steps that still operate on the
source step's own elements, stopping at element-changing steps such as
`VertexStep`/`EdgeVertexStep`/`PropertiesStep`, while keeping the walk alive
for `select()`/`path()`-shaped steps that can reintroduce earlier elements. If
that is follow-up work, please say in the FIXME above that a negative label
anywhere in the traversal disables all pushdown at every source step, so the
cost is recorded rather than implied to be narrow.
--
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]
