Copilot commented on code in PR #3034:
URL: https://github.com/apache/hugegraph/pull/3034#discussion_r3281008822
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +167,105 @@ public static void trySetGraph(Step<?, ?> step,
HugeGraph graph) {
public static void extractHasContainer(HugeGraphStep<?, ?> newStep,
Traversal.Admin<?, ?> traversal) {
- Step<?, ?> step = newStep;
- do {
- step = step.getNextStep();
+ Step<?, ?> step = newStep.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- if (!GraphStep.processHasContainerIds(newStep, has)) {
- newStep.addHasContainer(has);
- }
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- } while (step instanceof HasStep || step instanceof NoOpBarrierStep);
+ step = nextStep;
+ }
}
public static void extractHasContainer(HugeVertexStep<?> newStep,
Traversal.Admin<?, ?> traversal) {
Step<?, ?> step = newStep;
do {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- newStep.addHasContainer(has);
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- step = step.getNextStep();
+ step = nextStep;
} while (step instanceof HasStep || step instanceof NoOpBarrierStep);
}
+ private static boolean extractHasContainers(HugeGraphStep<?, ?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ if (!canExtractHasContainers(graph, hasContainers)) {
+ return false;
+ }
+ for (HasContainer has : hasContainers) {
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ }
+ return true;
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ if (!canExtractHasContainers(graph, hasContainers)) {
+ return false;
+ }
+ for (HasContainer has : hasContainers) {
+ newStep.addHasContainer(has);
+ }
+ return true;
+ }
+
+ private static boolean canExtractHasContainers(HugeGraph graph,
+ List<HasContainer>
hasContainers) {
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
The extraction is currently “all-or-nothing” per `HasStep`: if *any*
`HasContainer` is not extractable, `canExtractHasContainers()` returns false
and **none** of the other (safe) `HasContainer`s in the same `HasStep` get
extracted. This can break the intended behavior of “a text range filter should
not block later has extraction” in cases where Gremlin/TinkerPop merges
multiple `has()` constraints into a single `HasStep` (single holder with
multiple containers). A safer approach is to extract containers individually:
iterate `holder.getHasContainers()`, move only those where
`canExtractHasContainer(graph, has)` is true (and keep the rest on the
`HasStep`), then remove the `HasStep` only if it becomes empty (still copying
labels when removing).
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +167,105 @@ public static void trySetGraph(Step<?, ?> step,
HugeGraph graph) {
public static void extractHasContainer(HugeGraphStep<?, ?> newStep,
Traversal.Admin<?, ?> traversal) {
- Step<?, ?> step = newStep;
- do {
- step = step.getNextStep();
+ Step<?, ?> step = newStep.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- if (!GraphStep.processHasContainerIds(newStep, has)) {
- newStep.addHasContainer(has);
- }
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- } while (step instanceof HasStep || step instanceof NoOpBarrierStep);
+ step = nextStep;
+ }
}
public static void extractHasContainer(HugeVertexStep<?> newStep,
Traversal.Admin<?, ?> traversal) {
Step<?, ?> step = newStep;
do {
+ Step<?, ?> nextStep = step.getNextStep();
if (step instanceof HasStep) {
HasContainerHolder holder = (HasContainerHolder) step;
- for (HasContainer has : holder.getHasContainers()) {
- newStep.addHasContainer(has);
+ if (extractHasContainers(newStep, holder)) {
+ TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
+ traversal.removeStep(step);
}
- TraversalHelper.copyLabels(step, step.getPreviousStep(),
false);
- traversal.removeStep(step);
}
- step = step.getNextStep();
+ step = nextStep;
} while (step instanceof HasStep || step instanceof NoOpBarrierStep);
}
+ private static boolean extractHasContainers(HugeGraphStep<?, ?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ if (!canExtractHasContainers(graph, hasContainers)) {
+ return false;
+ }
+ for (HasContainer has : hasContainers) {
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ }
+ return true;
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ if (!canExtractHasContainers(graph, hasContainers)) {
+ return false;
+ }
+ for (HasContainer has : hasContainers) {
+ newStep.addHasContainer(has);
+ }
+ return true;
+ }
+
+ private static boolean canExtractHasContainers(HugeGraph graph,
+ List<HasContainer>
hasContainers) {
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ static boolean canExtractHasContainer(HugeGraph graph,
+ HasContainer has) {
+ if (isSysProp(has.getKey())) {
+ return true;
+ }
+ if (graph == null) {
+ return false;
+ }
Review Comment:
`canExtractHasContainer()` hard-rejects all non-sysprop containers when
`graph == null`, which forces the logic to keep the full `HasStep` even when
some containers might be extractable without schema access (for example,
id-based filtering handled by `GraphStep.processHasContainerIds`). Consider
special-casing extractable-without-schema keys/predicates (or delegating the
decision to the same logic that performs extraction), so the “graph is null”
case doesn’t unnecessarily disable safe optimizations.
--
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]
