bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3950916244
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +672,406 @@ 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(
Review Comment:
⚠️ **This fallback also removes the `NoIndexException` guard, so an
unindexed property query becomes a silent full scan**
Evidence: on this path the only things that reach the source step are
`~page` (line 691) and, under paging, a range bound (line 755). Every property
container stays in the local `HasStep`, so `HugeGraphStep.makeQuery()`
(`HugeGraphStep.java:146-151`) takes the `this.hasContainers.isEmpty()` branch
and builds `new Query(type)`. A plain `Query` never reaches
`GraphIndexTransaction.queryByUserprop()`, which is the only place
`noIndexException()` is raised.
At the merge base the same containers went through
`canExtractHasContainers()`, where `~label` passes via `isSysProp()` (line
1076) and the property predicate is folded into a `ConditionQuery`, so an
unindexed property failed fast.
So `g.V().has("unindexedProp", "x").has(T.label, P.neq("author"))` changes
from a fast `NoIndexException` to an unbounded full vertex scan filtered in
memory. This is the semantic half of the cost concern raised on 2026-09-06, not
a restatement of it: the issue is not that an indexed plan degrades, it is that
HugeGraph's refusal to run unindexed queries disappears for the whole class of
traversals this gate covers. Because the gate is traversal-wide, one non-EQ
label predicate anywhere in the chain, an ancestor, or a child traversal is
enough. The PR body documents reduced pushdown but not this, and `Doc - No
Need` is checked.
Requested change: either keep the fast failure (check index coverage for the
abandoned predicates and raise `NoIndexException` when none exists), or state
the new full-scan behaviour in the PR description and user docs and add a
regression that pins it.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/ram/RamTable.java:
##########
@@ -269,7 +269,7 @@ public boolean matched(Query query) {
int conditionsSize = cq.conditionsSize();
Object owner = cq.condition(HugeKeys.OWNER_VERTEX);
Directions direction = cq.condition(HugeKeys.DIRECTION);
- Id label = cq.condition(HugeKeys.LABEL);
+ Id label = cq.singleConditionValueOrNull(HugeKeys.LABEL);
Review Comment:
🧹 **`matched()` now declines multi-label edge queries that `query()` can
already serve**
Evidence: `singleConditionValueOrNull()` returns `null` for
`Condition.in(HugeKeys.LABEL, [a, b])`, so `matchedConds` reaches 2 while
`conditionsSize()` is 3 and `matched()` returns false.
`CachedGraphTransaction.java:393-395` then falls back to the backend for
`g.V(id).outE('a','b')` / `both('a','b')` — exactly the query
`GraphTransaction.constructEdgesQuery()` builds at line 1379.
This is strictly better than the merge base, where `Id label =
cq.condition(HugeKeys.LABEL)` compiled to a `checkcast Id` and threw
`ClassCastException` on the raw `IN` list, and
`RamTableTest#testMatchedLabelCandidateContract` pins the new behaviour. But
`query(Query)` at line 300 already calls `ConditionQueryFlatten.flatten()` and
resolves each label separately (it has to, for the `BOTH` direction `Or`), so
the in-memory adjacency fast path is capable of serving these queries and is
now simply skipped.
Requested change: accept a fully resolved multi-value label set in
`matched()` — for example `containsConditionValues(HugeKeys.LABEL)` with a
non-empty `conditionValues()` — and let the existing flatten in `query()`
handle the per-label lookups, rather than silently dropping the optimisation
for a common pattern.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +672,406 @@ 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())) {
+ // ID lookup is complete across labels. Existing source
+ // IDs and unsupported predicates still filter locally.
+ if (source instanceof HugeGraphStep &&
+ GraphStep.processHasContainerIds((HugeGraphStep<?,
?>) source, has)) {
+ holder.removeHasContainer(has);
+ continue;
+ }
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new
LocalIdHasContainer(has.getPredicate()));
+ continue;
+ }
+ if (T.label.getAccessor().equals(has.getKey())) {
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new
LocalLabelHasContainer(has.getPredicate()));
+ continue;
+ }
+ if (keyForContainsKey(has.getKey()) ||
keyForContainsValue(has.getKey())) {
+ // Backend CONTAINS support varies and source IDs use
+ // local filtering too. Preserve HugeGraph's map query
+ // semantics without treating "key"/"value" as names.
+ holder.removeHasContainer(has);
+ traversal.addStep(traversal.getSteps().indexOf(step),
+ new LocalContainsStep<>(traversal,
has));
+ 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 LocalIdHasContainer extends HasContainer {
+
+ private static final long serialVersionUID = 1L;
+
+ private LocalIdHasContainer(P<?> predicate) {
+ super(T.id.getAccessor(), localIdPredicate(predicate));
+ }
+
+ @Override
+ protected boolean testId(Element element) {
+ return testLocalIdPredicate(this.getPredicate(), element.id());
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ private static boolean testLocalIdPredicate(P<?> predicate, Object id) {
+ if (predicate instanceof ConnectiveP) {
+ boolean and = predicate instanceof AndP;
+ for (P<?> child : ((ConnectiveP<?>) predicate).getPredicates()) {
+ if (testLocalIdPredicate(child, id) != and) {
+ return !and;
+ }
+ }
+ return and;
+ }
+ Object value = predicate.getValue();
+ Object first = value;
+ if (value instanceof Collection) {
+ Collection<?> values = (Collection<?>) value;
+ first = values.isEmpty() ? null : values.iterator().next();
+ }
+ // HasContainer decides string-ID comparison from only the top-level
+ // P value. ConnectiveP has no such value; decide separately per leaf.
+ Object actual = first instanceof String ? id.toString() : id;
+ return ((BiPredicate<Object, Object>)
predicate.getBiPredicate()).test(actual, 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());
+ }
+ }
+
+ private static final class LocalContainsStep<S extends Element> extends
HasStep<S> {
+
+ private static final long serialVersionUID = 1L;
+
+ private LocalContainsStep(Traversal.Admin<?, ?> traversal,
HasContainer has) {
+ super(traversal, has.clone());
+ E.checkArgument(has.getPredicate().getBiPredicate() == Compare.eq,
+ "CONTAINS query with relation '%s' is not
supported",
+ has.getPredicate().getBiPredicate());
+ }
+
+ @Override
+ protected boolean filter(Traverser.Admin<S> traverser) {
+ HugeElement element = (HugeElement) traverser.get();
+ // Adjacent vertices can be ID/label-only shells. Like
properties(),
+ // load their properties before evaluating the system property map.
+ element.getFilledProperties();
+ // Keep a HasStep boundary so count/range cannot bypass this
filter.
+ // Resolve schema from the runtime element; clone/reset must not
+ // retain a graph or transaction captured during optimization.
+ for (HasContainer has : this.getHasContainers()) {
+ boolean matches = keyForContainsKey(has.getKey()) ||
keyForContainsValue(has.getKey()) ?
+ convContains2Relation(element.graph(),
has).test(element) :
+ has.test(element);
+ if (!matches) {
+ return false;
+ }
+ }
+ return true;
+ }
+ }
+
+ @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());
Review Comment:
🧹 **Wrapping the matcher in a lambda `P` loses the structure the rest of the
code reads off `P`**
```java
Predicate<Object> matcher = graph.searchPredicate((String)
predicate.getValue());
return new P<>((actual, ignored) -> matcher.test(actual),
predicate.getValue());
```
Three consequences:
- `P.getBiPredicate()` no longer reports
`Condition.RelationType.TEXT_CONTAINS`, so every later inspection of this
container (`isEqInLabelPredicate` line 968, `canExtractHasContainer` line 1074,
`hasMatchIndexSensitivePredicate`, `convHas2Condition`) misclassifies it.
Nothing re-walks this `HasStep` today, so it is latent rather than broken.
- `P.equals()` compares `biPredicate.equals(...)`, so this container's
equality and hash become identity-based and two structurally identical
traversals stop comparing equal through `HasStep.equals()`.
- The capturing lambda is not `Serializable`, unlike every other
`HasContainer` held by a `HasStep`.
Requested change: use the same shape as the two siblings on this path.
`LocalIdHasContainer` (line 787) and `LocalLabelHasContainer` (line 820) keep
the original `P` and override the element test instead; a
`LocalSearchHasContainer` overriding the value test would give identical
filtering without any of the three effects above.
--
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]
