Copilot commented on code in PR #3034:
URL: https://github.com/apache/hugegraph/pull/3034#discussion_r3281166043
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +168,110 @@ 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 = new
ArrayList<>(holder.getHasContainers());
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ continue;
+ }
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
Review Comment:
`canExtractHasContainer()` calls `hasIndexForProperty()`, which iterates
`graph.indexLabels()`; since this happens inside a loop over `HasContainer`s,
the extraction phase becomes O(#has * #indexLabels). If traversals commonly
carry multiple `has()` steps and graphs have many index labels, this could
become noticeable. A concrete improvement would be to precompute a `Set<Id>` of
indexed property key ids once per extraction pass (or cache
`graph.indexLabels()` / derived sets) and reuse it across all `HasContainer`s
being evaluated.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +168,110 @@ 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 = new
ArrayList<>(holder.getHasContainers());
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ continue;
+ }
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ continue;
+ }
+ newStep.addHasContainer(has);
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ static boolean canExtractHasContainer(HugeGraph graph,
+ HasContainer has) {
+ if (isSysProp(has.getKey())) {
+ return true;
+ }
+ if (graph == null) {
+ return false;
+ }
+
+ PropertyKey pkey;
+ try {
+ pkey = graph.propertyKey(has.getKey());
+ } catch (NotFoundException e) {
+ return false;
+ }
+ if (!hasIndexForProperty(graph, pkey)) {
+ return false;
+ }
+ if (!pkey.dataType().isText()) {
+ return true;
+ }
+
+ List<P<Object>> predicates = new ArrayList<>();
+ collectPredicates(predicates, ImmutableList.of(has.getPredicate()));
+ for (P<Object> pred : predicates) {
+ BiPredicate<?, ?> bp = pred.getBiPredicate();
+ if (bp == Compare.gt || bp == Compare.gte ||
+ bp == Compare.lt || bp == Compare.lte) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static boolean hasIndexForProperty(HugeGraph graph,
+ PropertyKey pkey) {
+ for (IndexLabel indexLabel : graph.indexLabels()) {
+ if (indexLabel.indexFields().contains(pkey.id())) {
+ return true;
+ }
+ }
+ return false;
+ }
Review Comment:
`hasIndexForProperty()` returns true if *any* index label in the graph
references the property key, without checking whether that index applies to the
current traversal scope (e.g., current vertex/edge label). If a property is
indexed on one label but queried on another label without that index, this can
incorrectly allow extraction and may lead to backend query planning errors or
invalid assumptions about index availability. Consider scoping this check to
the step’s target label(s) (e.g., by passing the relevant `SchemaLabel`/label
id(s) into `canExtractHasContainer()` and verifying `IndexLabel` base
type/value matches), or otherwise making the check conservative when label
context is known.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -166,38 +168,110 @@ 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 = new
ArrayList<>(holder.getHasContainers());
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ continue;
+ }
+ if (!GraphStep.processHasContainerIds(newStep, has)) {
+ newStep.addHasContainer(has);
+ }
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
+
+ private static boolean extractHasContainers(HugeVertexStep<?> newStep,
+ HasContainerHolder holder) {
+ HugeGraph graph = TraversalUtil.tryGetGraph(newStep);
+ List<HasContainer> hasContainers = new
ArrayList<>(holder.getHasContainers());
+ for (HasContainer has : hasContainers) {
+ if (!canExtractHasContainer(graph, has)) {
+ continue;
+ }
+ newStep.addHasContainer(has);
+ holder.removeHasContainer(has);
+ }
+ return holder.getHasContainers().isEmpty();
+ }
Review Comment:
These two `extractHasContainers()` overloads duplicate most of the same
control flow (copy list, filter by `canExtractHasContainer()`, remove from
holder, return emptiness) and differ mainly in the step-specific ‘add’ behavior
(`processHasContainerIds` vs direct add). Consider factoring out the shared
loop into a single helper that accepts an insertion strategy (e.g., a small
functional interface / lambda) to reduce duplication and keep future extraction
criteria changes consistent.
--
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]
