bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3896390088
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +651,55 @@ 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 boolean hasUnsafeLabelInChain(Step<?, ?> step) {
Review Comment:
⚠️ This chain scan is narrower than the loop it guards, so the "no partial
pushdown" invariant can still be bypassed.
`hasUnsafeLabelInChain()` walks only while `step instanceof HasStep || step
instanceof NoOpBarrierStep`, so it stops at the first `OrStep`/`IdentityStep`.
The loop it protects in `extractHasContainer(HugeGraphStep, …)` (line 177) does
not stop there: when `hasMatchIndexSensitivePredicate(holder)` is true,
`extractPositiveLabelOnlyOrStep()` folds a positive-label `OrStep` into
`newStep` and returns `orStep.getNextStep()` (lines 282-285), which the loop
assigns to `nextStep` and resumes from.
For a chain shaped like
```groovy
g.V().has("age", P.gt(1))
.or(__.hasLabel("a"), __.hasLabel("b"))
.has(T.label, P.neq("c"))
.has("city", "Beijing")
```
the guard returns `false` (it stops at the `OrStep`), the loop jumps past
the `OrStep`, `canExtractHasContainers()` correctly keeps `T.label neq c`
local, and the following `has("city", …)` is still extracted into
`HugeGraphStep`. That is the partial-pushdown shape the `FIXME` just above
declares unsafe, and it is the same control-flow gap the barrier pre-scan was
added to close.
I could not turn this into missing results — the folded `LABEL within [a,
b]` still bounds the candidates, and each flattened per-label branch raises
`NoIndexException` when a label lacks coverage — so this is an invariant gap
rather than proven data loss.
Requested change: make `hasUnsafeLabelInChain()` skip
`IdentityStep`/`OrStep` the way `positiveLabelOnlyOrStepAfter()` already does
(or re-run the guard whenever the loop jumps to `afterPositiveLabelOrStep`),
and add a `TraversalUtilOptimizeTest` case asserting
`newStep.getHasContainers().isEmpty()` for a positive-label `or(...)` followed
by a negative label container and an indexed property.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java:
##########
@@ -768,11 +772,17 @@ 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);
+ boolean hasLabelValues = query.containsConditionValues(HugeKeys.LABEL);
+ Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
List<? extends SchemaLabel> schemaLabels;
- if (label != null) {
- // Query has LABEL condition
+ if (hasLabelValues && labels.isEmpty()) {
Review Comment:
⚠️ An unsatisfiable label intersection reaches callers as "no index", not as
"no results".
This branch returns `Collections.emptySet()` when top-level `LABEL` EQ/IN
relations exist but resolve to an empty intersection. The only caller,
`queryByUserprop()`, cannot tell that apart from "no index matched" and throws
unconditionally (lines 487-491):
```java
Set<MatchedIndex> indexes = this.collectMatchedIndexes(query);
if (indexes.isEmpty()) {
Id label = query.singleConditionValueOrNull(HugeKeys.LABEL);
throw noIndexException(this.graph(), query, label);
}
```
An empty intersection means no element can satisfy the query, so the correct
outcome is an empty id list. Without this branch the query fell through to the
all-labels fallback and could still match an index, so the new branch actively
converts a correct empty result into an exception.
`testCollectMatchedIndexesByJointLabelsWithIndexedProperties` pins the empty
set down through `Whitebox`, but nothing covers what `queryByUserprop()` then
does with it.
For what it is worth, I could not reach this branch through
`optimizeQueries()` at this head: `ConditionQueryFlatten.optimizeRelations()`
drops conflicting EQ pairs in `mergeRelations()`, and `IN` is in
`RelationType.UNFLATTEN_TYPES`, so `queryIndex().checkFlattened()` rejects a
surviving `IN`. It reads as defensive today, but it is a trap for the next
caller.
Requested change: signal unsatisfiability distinctly instead of overloading
the empty `MatchedIndex` set — for example return
`IdHolderList.empty(query.paging())` early from `queryByUserprop()` when
`containsConditionValues(HugeKeys.LABEL)` holds and
`conditionValues(HugeKeys.LABEL)` is empty — and add a
`queryByUserprop()`-level regression alongside the existing `Whitebox` test.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/VertexCoreTest.java:
##########
@@ -54,6 +54,7 @@
import org.apache.hugegraph.exception.NotAllowException;
import org.apache.hugegraph.schema.PropertyKey;
import org.apache.hugegraph.schema.SchemaManager;
+import org.apache.hugegraph.schema.SchemaLabel;
Review Comment:
🧹 `SchemaLabel` is inserted after `SchemaManager`, which breaks the
alphabetical order the rest of this block follows (`PropertyKey`,
`SchemaManager`, `SchemaLabel`, `Userdata`, `VertexLabel`).
`style/checkstyle.xml:43` has the `ImportOrder` module commented out, so CI
will not flag it.
Requested change: move the import one line up, between `PropertyKey` and
`SchemaManager`.
--
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]
