bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r4075344184
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -173,6 +189,10 @@ public static void trySetGraph(Step<?, ?> step, HugeGraph
graph) {
public static void extractHasContainer(HugeGraphStep<?, ?> newStep,
Traversal.Admin<?, ?> traversal) {
+ if (hasUnsafeLabelInTraversal(traversal, newStep)) {
Review Comment:
⚠️ This fallback removes `NoIndexException` for a much wider set of
traversals than the negative-label case it is meant to protect.
`prepareLocalHasContainers()` (lines 690-770) only ever pushes down `~page`,
source IDs, and positive EQ/IN labels whose schema label has
`enableLabelIndex()`. Every ordinary property container stays in the local
`HasStep`, so the backend query carries no user-property condition,
`GraphIndexTransaction.queryIndex()` is never reached for those predicates, and
`noIndexException()` can never fire. `g.V().has("unindexedProp", "x")` raises
`NoIndexException` today; `g.V().has("unindexedProp",
"x").hasLabel(P.neq("author"))` becomes a candidate scan that runs to the
capacity limit or to completion.
The PR body and `docs/negative-label-queries.md` call this out as
intentional and `FIXME(#3201)` at line 1058 tracks narrowing it, so the
behavior itself is disclosed. The concern is the blast radius of the gate:
`hasUnsafeLabelInTraversal()` scans the remaining steps, every child traversal,
*and* each ancestor's remaining steps, so one downstream negative label
anywhere disables property pushdown at the source.
`g.V().has("city","x").where(__.out().hasLabel(P.neq("y")))` returns true from
the gate even though the negative label filters `out()`'s output and cannot
affect the source step's candidate set - `WhereTraversalStep` is not in the
`onlyCurrentElementSuffix()` allowlist, so the early exit at line 1066 does not
apply.
An operator upgrading into this gets a fast, actionable index error replaced
by a long scan or a capacity exception, with no way to opt out.
Requested change: gate the missing-index fallback behind an explicit config
option that defaults to today's `NoIndexException` behavior, or keep throwing
`NoIndexException` when no candidate schema label has index coverage for the
extracted property predicates. At minimum, narrow the ancestor/child scan so a
negative label that applies only to a downstream element does not disable
pushdown at the source step.
##########
docs/negative-label-queries.md:
##########
@@ -0,0 +1,64 @@
+# Negative-label queries and local filtering
Review Comment:
🧹 This note is an orphan in this repository.
`docs/` here holds contributor docs (`BUILDING.md`, `CONTRIBUTING.md`), not
user-facing query semantics. `git grep negative-label-queries` at this head
returns no hits, so no code, README or test links to it, while the PR
description points readers at a gist as the authoritative copy and marks
Documentation Status as `Doc - TODO`. That leaves three copies of the contract
(gist, this file, the pending website change) and no way for a reader of
`TraversalUtil` to find any of them.
Requested change: either reference it from the code that implements the
behavior (a `@see` or a comment on `TraversalUtil.prepareLocalHasContainers`)
plus the relevant README, or drop it from this PR and land the content in
`apache/hugegraph-doc` so there is a single source of truth.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -768,11 +771,12 @@ private PageIds doIndexQueryOnce(IndexLabel indexLabel,
@Watched(prefix = "index")
private Set<MatchedIndex> collectMatchedIndexes(ConditionQuery query) {
ISchemaTransaction schema = this.params().schemaTransaction();
- Id label = query.condition(HugeKeys.LABEL);
+ Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
List<? extends SchemaLabel> schemaLabels;
- if (label != null) {
- // Query has LABEL condition
+ if (labels.size() == 1) {
Review Comment:
‼️ An empty LABEL intersection falls into the "no LABEL condition" branch
and enumerates every schema label, and this PR's own regression test fails
because of it.
Branching only on `labels.size() == 1` leaves three distinct cases sharing
the `else` arm: no EQ/IN LABEL relation at all, a multi-label query, and LABEL
relations that resolve to an *empty* intersection. Only the first two should
widen to all schema labels.
The previously reviewed head `bcb8c1f3` had the guard here; the `a07f6aae`
merge removed it:
```java
boolean hasLabelValues = query.containsConditionValues(HugeKeys.LABEL);
Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
List<? extends SchemaLabel> schemaLabels;
if (hasLabelValues && labels.isEmpty()) {
// LABEL EQ/IN conditions resolve to an empty intersection.
return Collections.emptySet();
}
```
The test that asserts the contract was kept.
`VertexCoreTest#testCollectMatchedIndexesByJointLabelsWithIndexedProperties`
(VertexCoreTest.java:9741-9756) builds `LABEL eq person` AND `LABEL eq
computer` plus `city eq "Beijing"`, asserts `conditionValues(LABEL)` is empty,
then asserts `collectMatchedIndexes` returns 0. It now returns 1 (`person`'s
`city` index):
```
[ERROR]
VertexCoreTest.testCollectMatchedIndexesByJointLabelsWithIndexedProperties:9756
expected:<0> but was:<1>
[INFO] BUILD FAILURE
```
This fails on `build-server (memory, 11)` (job 106872307016), `build-server
(rocksdb, 11)` (job 106872306860), `hstore` (job 106844898811) and both
`build-server-macos-rocksdb` jobs at this exact head, and it is the only test
failure in those runs.
The guard relocated to `queryByUserprop()` (lines 482-486) does not cover
this method: `IndexLeftJob.findMatchedIndexLabel()` (line 1863) calls
`collectMatchedIndexes` directly. That caller always passes a flattened query
today, so the practical impact is limited, but silently widening a
conflicting-label query to every schema label is exactly the
absent/empty/single/multi conflation this PR sets out to remove.
Requested change: restore the `hasLabelValues && labels.isEmpty()` early
return here (keeping the `java.util.Collections` usage) and keep the
`queryByUserprop` guard as well. If the intent really is that only
`queryByUserprop` owns this case, then VertexCoreTest.java:9756 has to be
updated with a comment explaining why an empty intersection may match every
label - but restoring the guard is the smaller fix and it is what turns CI
green.
--
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]
