bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3972183209
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQuery.java:
##########
@@ -323,20 +310,151 @@ public <T> T condition(Object key) {
return value;
}
+ /**
+ * Returns whether there is any top-level relation for the specified key.
+ */
+ public boolean containsCondition(Object key) {
Review Comment:
🧹 This overload is public but has one caller in the tree, and it widens an
overload set whose members disagree about what the argument means.
`git grep 'containsCondition('` at this head finds a single call to it: line
407, inside `containsCondition(HugeKeys)`. The set now reads:
- `containsCondition(Object key)` here and `containsCondition(HugeKeys key)`
at line 406: is there a top-level relation **on this key**
- `containsCondition(Condition.RelationType type)` at line 460: is there a
top-level relation **using this operator**
Both pre-existing overloads take enums, so no other argument type compiled.
With `Object` in the set, any reference type does and silently resolves to key
matching, including `Condition.Relation.key()`, which is declared `Object`
(`Condition.java:739`).
Requested change: make this method `private`. If a public key accessor is
wanted for `Id` userprop keys, give it a distinct name such as
`containsConditionKey(Object)`, matching the `containsConditionValues` naming
you introduced, rather than another overload. `containsConditionValues(Object)`
at line 350 has the same single-delegate-caller shape but no competing
overload, so it is only extra surface.
##########
docs/negative-label-queries.md:
##########
@@ -0,0 +1,53 @@
+# Negative-label queries and local filtering
Review Comment:
🧹 Two fork URLs in the PR description point at this content and will break
on merge.
Description line 98 links
`github.com/contrueCT/hugegraph/blob/e32a75ff.../docs/negative-label-queries.md`,
and line 37 embeds
`raw.githubusercontent.com/contrueCT/hugegraph/b3f5642.../docs/images/pr-2994-condition-resolution.png`.
Both die once that branch is deleted, and they are what a reader following the
merged commit lands on. Requested change: point the link at the in-repo path,
and either commit the diagram under `docs/images/` or drop it.
Separately, `docs/` at this head is `BUILDING.md`, `CONTRIBUTING.md` and
this file, so it is a contributor-docs folder; HugeGraph user documentation is
published from `apache/hugegraph-doc`. What this file describes is
user-visible: `g.V().has("unindexedProp", "x")` still raises `NoIndexException`
while the same query followed by `hasLabel(P.neq("author"))` now scans
candidates, and the paging note changes what a correct client has to do. Once
the `SEARCH predicates` section is settled on the other open thread, a
follow-up in `hugegraph-doc` would put this in front of the users who hit it,
which is what the checked `Doc - Done` box implies for an API-affecting change.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +674,533 @@ 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);
+ HasContainer label = new
LocalLabelHasContainer(has.getPredicate());
+ if (source instanceof HugeGraphStep &&
+ canPushPositiveLabel((HugeGraphStep<?, ?>) source,
has)) {
+ // A positive label conjunct uses the label index,
+ // independent of per-label property index
coverage.
+ // Keep the runtime label matcher for source-ID
queries.
+ query.addHasContainer(label);
+ } else {
+ // Keep unsupported candidates and adjacent-vertex
+ // labels local, including their paging boundary.
+ holder.addHasContainer(label);
+ }
+ 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)) {
+ // Child traversals can be unbound during optimization;
+ // the matcher resolves the graph from each runtime
element.
+ 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 boolean canPushPositiveLabel(HugeGraphStep<?, ?> source,
+ HasContainer has) {
+ if (!isEqInLabelPredicate(has)) {
+ return false;
+ }
+ HugeGraph graph = tryGetGraph(source);
+ if (graph == null) {
+ return false;
+ }
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates, ImmutableList.of(has.getPredicate()));
+ for (P<Object> predicate : predicates) {
+ Object value = predicate.getValue();
+ BiPredicate<?, ?> bp = predicate.getBiPredicate();
+ if (bp == Contains.within && !(value instanceof Collection)) {
+ return false;
+ }
+ Collection<?> values = bp == Contains.within ?
+ (Collection<?>) value :
Collections.singletonList(value);
+ for (Object candidate : values) {
+ if (!hasLabelIndex(graph, source.returnsVertex(), candidate)) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }
+
+ private static boolean hasLabelIndex(HugeGraph graph, boolean vertex,
Object value) {
+ if (value instanceof Number) {
+ value = IdGenerator.of(((Number) value).longValue());
+ }
+ try {
+ SchemaLabel label;
+ if (value instanceof Id) {
+ Id id = (Id) value;
+ // Nonpositive IDs include internal schema objects that aren't
+ // user labels. Preserve their local matching behavior.
+ if (!id.number() || id.asLong() <= 0L) {
+ return false;
+ }
+ label = vertex ? graph.vertexLabel(id) : graph.edgeLabel(id);
+ } else if (value instanceof String) {
+ label = vertex ? graph.vertexLabel((String) value) :
graph.edgeLabel((String) value);
+ } else {
+ return false;
+ }
+ // Do not turn a working local filter into a missing-label/index
error.
+ return label != null && label.enableLabelIndex();
+ } catch (IllegalArgumentException | NotFoundException e) {
+ return false;
+ }
+ }
+
+ 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 final class LocalSearchHasContainer extends HasContainer {
+
+ private static final long serialVersionUID = 1L;
+
+ private transient HugeGraph matcherGraph;
+ private transient P<?> matcherPredicate;
+ private transient Predicate<Object> matcher;
+
+ private LocalSearchHasContainer(String key, P<?> predicate) {
+ super(key, predicate.clone());
+ }
+
+ @Override
+ protected boolean testValue(Property property) {
+ // Keep the public P tree intact for strategies, hashing and Java
+ // serialization. Resolve the analyzer from the element's graph,
+ // including after cloning, deserialization or graph rebinding.
+ HugeGraph graph = (HugeGraph) property.element().graph();
+ if (this.matcherGraph != graph ||
+ !samePredicateValues(this.getPredicate(),
this.matcherPredicate)) {
+ P<?> predicate = this.getPredicate().clone();
+ this.matcher = localSearchMatcher(predicate, graph);
+ this.matcherPredicate = predicate;
+ this.matcherGraph = graph;
+ }
+ return this.matcher.test(property.value());
+ }
+
+ @Override
+ public LocalSearchHasContainer clone() {
+ LocalSearchHasContainer clone = (LocalSearchHasContainer)
super.clone();
+ clone.matcherGraph = null;
+ clone.matcherPredicate = null;
+ clone.matcher = null;
+ return clone;
+ }
+ }
+
+ private static boolean samePredicateValues(P<?> current, P<?> cached) {
+ // P.equals() compares originalValue, not the value changed by
setValue().
+ if (cached == null || current.getClass() != cached.getClass()) {
+ return false;
+ }
+ if (current instanceof ConnectiveP) {
+ List<? extends P<?>> children = ((ConnectiveP<?>)
current).getPredicates();
+ List<? extends P<?>> oldChildren = ((ConnectiveP<?>)
cached).getPredicates();
+ if (children.size() != oldChildren.size()) {
+ return false;
+ }
+ for (int i = 0; i < children.size(); i++) {
+ if (!samePredicateValues(children.get(i), oldChildren.get(i)))
{
+ return false;
+ }
+ }
+ return true;
+ }
+ return current.getBiPredicate().equals(cached.getBiPredicate()) &&
+ Objects.equals(current.getValue(), cached.getValue());
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Predicate<Object> localSearchMatcher(P<?> predicate,
HugeGraph graph) {
+ if (predicate instanceof ConnectiveP) {
+ List<Predicate<Object>> children = new ArrayList<>();
+ for (P<?> child : ((ConnectiveP<?>) predicate).getPredicates()) {
+ children.add(localSearchMatcher(child, graph));
+ }
+ boolean and = predicate instanceof AndP;
+ return value -> {
+ for (Predicate<Object> child : children) {
+ if (child.test(value) != and) {
+ return !and;
+ }
+ }
+ return and;
+ };
+ }
+ if (predicate.getBiPredicate() !=
Condition.RelationType.TEXT_CONTAINS) {
+ return (P<Object>) predicate;
+ }
+ // Match SEARCH terms in place, preserving range and side-effect
ordering.
+ return graph.searchPredicate((String) 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
Review Comment:
🧹 This `FIXME` marks the fallback as provisional but gives a reader no issue
to follow.
It is the one tracked exit from the conservative behaviour the rest of the
PR builds on: while it stands, a downstream unsafe label predicate disables
property pushdown across element changes, ancestors and unknown extension
steps, which is exactly the full-scan cost `docs/negative-label-queries.md`
warns about. You already split the `convContains2Relation` caching out to
#3196, so the convention is established here.
Requested change: file an issue for restoring selective pushdown once every
candidate schema label can be shown to have compatible index coverage, and put
its number on this line, `// FIXME(#NNNN): ...`, so the fallback has a tracked
path back.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQuery.java:
##########
@@ -323,20 +310,151 @@ public <T> T condition(Object key) {
return value;
}
+ /**
+ * Returns whether there is any top-level relation for the specified key.
+ */
+ public boolean containsCondition(Object key) {
+ for (Condition c : this.conditions) {
+ if (c.isRelation()) {
+ Condition.Relation r = (Condition.Relation) c;
+ if (r.key().equals(key)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Returns the resolved candidate values of the specified key from
+ * top-level EQ/IN relations.
+ *
+ * Use {@link #containsConditionValues(Object)} to distinguish "no EQ/IN
+ * condition" from "EQ/IN conditions exist but resolve to an empty
+ * intersection".
+ */
+ public Set<Object> conditionValues(Object key) {
+ List<Object> valuesEQ = InsertionOrderUtil.newList();
+ List<Object> valuesIN = InsertionOrderUtil.newList();
+ this.collectConditionValues(key, valuesEQ, valuesIN);
+ if (valuesEQ.isEmpty() && valuesIN.isEmpty()) {
+ return InsertionOrderUtil.newSet();
+ }
+ return this.resolveConditionValues(valuesEQ, valuesIN);
+ }
+
+ /**
+ * Returns whether there is any top-level EQ/IN relation for the specified
+ * key.
+ */
+ public boolean containsConditionValues(Object key) {
+ for (Condition c : this.conditions) {
+ if (c.isRelation()) {
+ Condition.Relation r = (Condition.Relation) c;
+ if (r.key().equals(key) &&
+ (r.relation() == RelationType.EQ ||
+ r.relation() == RelationType.IN)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Returns the unique resolved value of the specified key from top-level
+ * EQ/IN relations.
+ *
+ * Returns {@code null} when the resolved candidate set is empty. Throws
+ * if multiple values remain after resolution.
+ */
+ public <T> T conditionValue(Object key) {
+ Set<Object> values = this.conditionValues(key);
+ if (values.isEmpty()) {
+ return null;
+ }
+ E.checkState(values.size() == 1,
+ "Illegal key '%s' with more than one value: %s",
+ key, values);
+ @SuppressWarnings("unchecked")
+ T value = (T) values.iterator().next();
+ return value;
+ }
+
+ /**
+ * Returns the unique resolved value of the specified key from top-level
+ * EQ/IN relations, or {@code null} if the resolved candidate set doesn't
+ * contain exactly one value.
+ *
+ * Use this method when callers want "single-or-null" semantics instead of
+ * treating multiple remaining values as an error.
+ */
+ public <T> T singleConditionValueOrNull(Object key) {
+ Set<Object> values = this.conditionValues(key);
+ if (values.size() != 1) {
+ return null;
+ }
+ @SuppressWarnings("unchecked")
+ T value = (T) values.iterator().next();
+ return value;
+ }
+
public void unsetCondition(Object key) {
this.conditions.removeIf(c -> c.isRelation() && ((Relation)
c).key().equals(key));
}
public boolean containsCondition(HugeKeys key) {
+ return this.containsCondition((Object) key);
+ }
+
+ public boolean containsConditionValues(HugeKeys key) {
+ return this.containsConditionValues((Object) key);
+ }
+
+ private void collectConditionValues(Object key, List<Object> valuesEQ,
+ List<Object> valuesIN) {
for (Condition c : this.conditions) {
if (c.isRelation()) {
Condition.Relation r = (Condition.Relation) c;
if (r.key().equals(key)) {
- return true;
+ if (r.relation() == RelationType.EQ) {
+ valuesEQ.add(r.value());
+ } else if (r.relation() == RelationType.IN) {
+ Object value = r.value();
+ assert value instanceof List;
+ valuesIN.add(value);
+ }
}
}
}
- return false;
+ }
+
+ private Set<Object> resolveConditionValues(List<Object> valuesEQ,
Review Comment:
🧹 A second `ConditionQuery` in this repo keeps the legacy semantics, and its
copy of this loop drops conflicts.
`hugegraph-struct/src/main/java/org/apache/hugegraph/query/ConditionQuery.java`
carries a near-duplicate `condition(Object)` (lines 463-524 at this head). It
is the class the store side deserializes:
`hg-store-common/.../StoreQueryParam.java`,
`hg-store-core/.../business/FilterIterator.java` (`ConditionQuery.fromBytes`,
line 49) and `hg-store-node/.../grpc/query/stages/FilterStage.java` all import
`org.apache.hugegraph.query.ConditionQuery`.
Its intersection loop (lines 494-513) gates on `intersectValues.isEmpty()`
instead of the `initialized` flag this method uses, so an emptied set is read
as "not seeded yet". For three conflicting relations on one key, `EQ a, EQ b,
EQ c`, the walk is `{a}`, then intersect `[b]` gives `{}`, then the third
iteration takes the seed branch and re-seeds `{c}`; it returns `c` where this
file returns `null`. Two conflicting relations happen to coincide, which is why
it is easy to miss. The struct copy also has none of the explicit accessors you
added here.
Nothing is broken today: `git grep '\.condition('` over `hugegraph-struct`
and `hugegraph-store` finds only `Condition.java:770` and
`ConditionQuery.java:440`, so there is no production caller. This PR is scoped
to `server`, so I am not asking you to widen it.
Requested change: open a follow-up to port
`collectConditionValues`/`resolveConditionValues` and the explicit accessors to
the struct copy, and add a short comment on each copy naming the other so the
two do not drift further.
--
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]
