bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3955850524
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +673,472 @@ 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())) {
Review Comment:
⚠️ **Positive label containers are demoted too, so a label-index lookup
becomes a full scan**
Every `T.label` container reaching this branch becomes a
`LocalLabelHasContainer` and stays in the `HasStep`, including a plain
`hasLabel("person")` with a `Compare.eq` predicate. When it is the only
container on the chain, nothing reaches the source step, so
`HugeGraphStep.makeQuery()` (HugeGraphStep.java:147-155) takes the
`hasContainers.isEmpty()` branch and builds `new Query(VERTEX)`.
`g.V().hasLabel("person").where(__.out().hasLabel(P.neq("software")))` lands
there: `TraversalFilterStep` is not element-changing, so the escape at line
1009 never fires and `hasUnsafeLabelInChildren()` at line 1020 reports the
child's negative label. At the merge base that label was extracted and served
by `GraphIndexTransaction.queryByLabel()`; now the source scans every vertex,
although the negative label only filters the `out()` results.
The FIXME at 996-1000 covers property pushdown, but a positive label looks
separable from it: it is an AND conjunct on the same element, and
`queryByLabel()` reads the built-in `VERTEX_LABEL_INDEX` rather than a
per-label property index, so pushing it cannot drop candidates the later
predicate still needs.
Requested change: keep pushing EQ/IN label containers into the source step
here, and demote only unsafe label predicates and their sibling property
predicates.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +673,472 @@ 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) {
Review Comment:
🧹 **The new doc promises analyzer matching that this branch does not give**
`docs/negative-label-queries.md` states without qualification that local
`Text.contains()` filters use the graph's SEARCH analyzer and exact term
matcher. On this branch the container keeps
`Condition.RelationType.TEXT_CONTAINS`, whose tester is `((String)
v1).contains((String) v2)` (Condition.java:94-97), so
`Text.contains("(alpha)")` looks for the literal substring `(alpha)`,
parentheses included. The behaviour itself predates the PR (at the merge base
`canExtractHasContainer` returned false on `graph == null` and left the same
raw container behind), but the doc shipped here now describes the analyzed path
as the rule.
Worth noting the gate is not what keeps the rewrite working:
`LocalSearchHasContainer` takes no graph at construction (lines 914-916) and
resolves one from `property.element().graph()` in `testValue` (lines 918-932),
so installing it unconditionally would not reintroduce the `getGraph()` failure
raised earlier in review.
Requested change: either drop the `tryGetGraph` gate so both cases match the
doc, or qualify the doc's SEARCH paragraph and say here that matching degrades
to substring on this path.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +673,472 @@ 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 LocalSearchHasContainer(
+ has.getKey(), has.getPredicate()));
+ }
+ }
+ 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) :
Review Comment:
🧹 **`convContains2Relation()` is rebuilt for every element**
This runs once per traverser. For `hasKey(...)` with a String value,
`convContains2Relation()` (lines 1554-1574) resolves `graph.propertyKey(name)`
before allocating a fresh `Condition`, so that is a schema-cache hit plus an
allocation per candidate rather than per query. On the `HugeGraphStep`
local-filter path the candidate set is whatever the source scan produces, which
is where it adds up.
Requested change: resolve the `Condition` once and cache it against the
element's graph, the way `LocalSearchHasContainer` caches its matcher, instead
of rebuilding it inside the filter loop.
--
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]
