bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3910063816
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +658,107 @@ 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,
+ boolean
followPositiveLabelOr) {
+ // Partial pushdown can lose candidates before local label filtering.
+ // FIXME: Restore selective pushdown when every candidate schema label
+ // has compatible index coverage for extracted property predicates.
+ while (step instanceof HasStep ||
+ step instanceof NoOpBarrierStep ||
+ step instanceof RangeGlobalStep ||
+ step instanceof IdentityStep) {
Review Comment:
‼️ Widening this whitelist again will not close the class. Any step
TinkerPop will not hoist a `has()` across leaves a later negative-label
predicate unseen while the earlier property filter is already extracted into
`HugeGraphStep`.
`FilterRankingStrategy` hoists a trailing `has()` above `or()`, `order()`
and `dedup()`, which is why those shapes behave. It leaves rank-0 steps alone:
side effects such as `aggregate()`, `store()` and `sideEffect()`, and filters
such as `coin()` and `sample()`.
Reproduced at this head on the memory backend with the
`testQueryByIndexedPropertyAndNonEqLabel` fixture (`initPersonIndex(true)`,
`init5Persons()`, plus a `fan` vertex with `city=Beijing` and no city index):
```
g.V().has("city","Beijing").aggregate("x").has(T.label, P.neq("author"))
[HugeGraphStep(Vertex,[city.eq(Beijing)]), AggregateGlobalStep(x),
HasStep([~label.neq(author)])]
.values("name") -> [James, Tom Cat, Lisa]
g.V().has("city","Beijing").has(T.label, P.neq("author"))
.values("name") -> [unindexed-city-fan, James, Tom Cat, Lisa]
```
`aggregate("x")` is a pure side effect, so both should return the same four
names. `coin(1.0)` in place of `aggregate("x")` gives the same three. Master
has no guard at all, so this is a gap in the new guard and not a regression.
Requested change: express the condition over the traversal rather than over
a step list, so pushdown is refused whenever an unsafe label predicate can
still apply to the same elements downstream, and leave the whitelist only for
the steps the extraction loop itself crosses. `TraversalUtilOptimizeTest` line
435 asserts the opposite for the `HugeVertexStep` path (`age` extracted with a
trailing `~label.neq`); that chain is unreachable once strategies run, but it
pins the invariant the wrong way round. Please add `VertexCoreTest` and
`EdgeCoreTest` regressions with `aggregate("x")` and `coin(1.0)` between the
indexed property and the negative label.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BinarySerializer.java:
##########
@@ -763,6 +763,17 @@ private Query writeQueryEdgePrefixCondition(ConditionQuery
cq) {
return null;
}
+ private Object edgeIdConditionValue(ConditionQuery cq, HugeKeys key) {
Review Comment:
🧹 `edgeIdConditionValue()` is duplicated character for character in
`TextSerializer` (line 519), and both classes extend `AbstractSerializer`. The
same consolidation argument was already made on this PR for `uniqueLabel()`,
which ended up folded into `ConditionQuery.singleConditionValueOrNull()`.
Requested change: move the helper into `AbstractSerializer` as a `protected`
method and call it from both serializers.
--
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]
