imbajin commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3723054528
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -1230,22 +1371,39 @@ private static void collectPredicates(List<P<Object>>
results,
private static Object convSysValueIfNeeded(HugeGraph graph,
HugeType type,
HugeKeys key,
- Object value) {
+ Object value,
+ boolean allowUndefinedLabel) {
if (key == HugeKeys.LABEL && !(value instanceof Id)) {
+ if (allowUndefinedLabel && value instanceof String &&
+ !existsLabel(graph, type, (String) value)) {
+ return value;
Review Comment:
‼️ Returning an undefined negative label as a plain `String` makes it
compare equal to any schema `Id` whose `asString()` matches. If schema label
`foo` has numeric id `1` and no label named `1` exists, `has(T.label,
P.neq("1"))` preserves `"1"`; `Condition.RelationType.equals()` then treats
`foo`'s `Id(1)` as equal and the new `negativeLabelCandidates()` path silently
excludes it. Please keep undefined names distinct from `Id` values (for example
with a typed sentinel) and add vertex/edge regressions where an unknown label
name equals an existing numeric label ID.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +654,139 @@ 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) {
+ // Partial pushdown can lose candidates before local label filtering.
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep &&
+ hasUnsafeLabelPredicate((HasContainerHolder) step)) {
+ return true;
+ }
+ step = step.getNextStep();
+ }
+ return false;
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeGraphStep<?, ?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeVertexStep<?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ newStep.addHasContainer(has);
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static boolean canExtractMixedNegativeLabelChain(
+ HugeGraph graph, Step<?, ?> step) {
+ boolean hasNegativeLabel = false;
+ boolean hasUserprop = false;
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (isNegativeLabelContainer(has)) {
+ hasNegativeLabel = true;
+ continue;
+ }
+ if (!isSysProp(has.getKey())) {
+ hasUserprop = true;
+ }
+ if (!canExtractHasContainer(graph, has)) {
+ return false;
+ }
+ }
+ }
+ step = step.getNextStep();
+ }
+ return hasNegativeLabel && hasUserprop;
+ }
+
+ private static boolean isNegativeLabelContainer(HasContainer has) {
+ if (!has.getKey().equals(T.label.getAccessor())) {
+ return false;
+ }
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates, ImmutableList.of(has.getPredicate()));
+ for (P<Object> predicate : predicates) {
+ BiPredicate<?, ?> bp = predicate.getBiPredicate();
+ if (bp != Compare.neq && bp != Contains.without) {
Review Comment:
‼️ `collectPredicates()` flattens connective structure, so
`P.neq("A").or(P.neq("B"))` is classified as a safe negative-label container.
With a property predicate, mixed extraction pushes it into `ConditionQuery`;
`ConditionQueryFlatten` creates one query per OR branch and `QueryList.fetch()`
flat-maps them without cross-query deduplication, so a label C matching both
branches is returned twice and `count`/`limit`/`range` become incorrect. Please
require a negative conjunction here, or preserve negative OR predicates for
local filtering, and add vertex/edge list/count/limit coverage.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +654,139 @@ 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) {
+ // Partial pushdown can lose candidates before local label filtering.
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep &&
+ hasUnsafeLabelPredicate((HasContainerHolder) step)) {
+ return true;
+ }
+ step = step.getNextStep();
+ }
+ return false;
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeGraphStep<?, ?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeVertexStep<?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ newStep.addHasContainer(has);
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static boolean canExtractMixedNegativeLabelChain(
+ HugeGraph graph, Step<?, ?> step) {
+ boolean hasNegativeLabel = false;
+ boolean hasUserprop = false;
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (isNegativeLabelContainer(has)) {
+ hasNegativeLabel = true;
+ continue;
+ }
+ if (!isSysProp(has.getKey())) {
+ hasUserprop = true;
+ }
+ if (!canExtractHasContainer(graph, has)) {
Review Comment:
‼️ The mixed-negative extraction checks only `canExtractHasContainer()`
here, so it accepts index-sensitive range predicates without verifying a usable
index. In the exact-head traversal `E().has("ep4",
P.lt(...)).and(__.hasLabel(P.neq("el2")))`, this path pushes `~label` into
`HugeGraphStep`;
`CountStrategyCoreTest#testNegativeConnectiveLabelAfterNoIndexRangeStaysLocal`
fails at line 536 on memory, RocksDB, HBase, both macOS RocksDB jobs, and
HStore. Please gate this path with the existing
`hasUsableMatchIndex()`/`hasUnusableMatchPredicate()` checks and keep the
negative connective label local when the range predicate has no usable index.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +654,139 @@ 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) {
+ // Partial pushdown can lose candidates before local label filtering.
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep &&
+ hasUnsafeLabelPredicate((HasContainerHolder) step)) {
+ return true;
+ }
+ step = step.getNextStep();
+ }
+ return false;
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeGraphStep<?, ?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static void extractMixedNegativeLabelChain(
+ HugeVertexStep<?> newStep, Traversal.Admin<?, ?> traversal,
+ Step<?, ?> step) {
+ HugeGraph graph = tryGetGraph(newStep);
+ if (!canExtractMixedNegativeLabelChain(graph, step)) {
+ return;
+ }
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
+ if (step instanceof HasStep) {
+ removeConnectiveLabelStep(step);
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ newStep.addHasContainer(has);
+ }
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
+ }
+ step = nextStep;
+ }
+ }
+
+ private static boolean canExtractMixedNegativeLabelChain(
+ HugeGraph graph, Step<?, ?> step) {
+ boolean hasNegativeLabel = false;
+ boolean hasUserprop = false;
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : holder.getHasContainers()) {
+ if (isNegativeLabelContainer(has)) {
+ hasNegativeLabel = true;
+ continue;
+ }
+ if (!isSysProp(has.getKey())) {
+ hasUserprop = true;
+ }
+ if (!canExtractHasContainer(graph, has)) {
+ return false;
+ }
+ }
+ }
+ step = step.getNextStep();
+ }
+ return hasNegativeLabel && hasUserprop;
+ }
+
+ private static boolean isNegativeLabelContainer(HasContainer has) {
+ if (!has.getKey().equals(T.label.getAccessor())) {
+ return false;
+ }
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates, ImmutableList.of(has.getPredicate()));
+ for (P<Object> predicate : predicates) {
+ BiPredicate<?, ?> bp = predicate.getBiPredicate();
+ if (bp != Compare.neq && bp != Contains.without) {
Review Comment:
⚠️ The same predicate test accepts `P.without()` with an empty value list.
That is a tautology, but `convNotin2And()` returns null for an empty `NOT_IN`
and `ConditionQueryFlatten.flatten()` then returns zero queries, so
`g.V().has(T.label, P.without()).has("city", "Beijing")` can become an empty
query/assert instead of the city filter. Please treat empty `WITHOUT` as a
true/no-op condition or keep it local, and cover vertex/edge ordering and
barrier cases.
--
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]
