From 39e3a74296ed13d0011d50f0b0234061d8808af9 Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Tue, 11 Apr 2023 08:53:50 -0400 Subject: [PATCH] TINKERPOP-2911 Fixed bug in CountStrategy around ConnectiveStep A previous attempt was made at fixing issues around this pattern but it turned out insufficient and needed its own special handling. Added a few other tests as well semi-unrelated to this issue to improve coverage. --- CHANGELOG.asciidoc | 3 +- .../strategy/optimization/CountStrategy.java | 32 ++++++++++++++----- .../optimization/CountStrategyTest.java | 7 ++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index fc18243d55c..e36e9ccf345 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -24,7 +24,8 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima === TinkerPop 3.5.6 (Release Date: NOT OFFICIALLY RELEASED YET) * Added `GraphFilter` support to `GraphSONRecordReader`. -* gremlin-python aiohttp dependency requirement upper bound relaxed to <4.0.0 +* gremlin-python aiohttp dependency requirement upper bound relaxed to <4.0.0. +* Fixed bug in `CountStrategy` where `or()` and `and()` filters were not being rewritten properly for some patterns. * Changed `PartitionStrategy` to force its filters to the end of the chain for `Vertex` and `Edge` read steps, thus preserving order of the `has()`. * Added `RequestOptions` and `RequestOptionsBuilder` types to Go GLV to encapsulate per-request settings and bindings. * Improved `addE()` error messaging when traverser is not a `Vertex`. diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java index 3def9691473..d4cc525641f 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategy.java @@ -140,14 +140,17 @@ public void apply(final Traversal.Admin traversal) { traversal.asAdmin().removeStep(curr); // CountStep size -= 2; if (!dismissCountIs) { - final TraversalParent p; - if ((p = traversal.getParent()) instanceof FilterStep && !(p instanceof ConnectiveStep)) { - final Step filterStep = parent.asStep(); - final Traversal.Admin parentTraversal = filterStep.getTraversal(); - final Step notStep = new NotStep<>(parentTraversal, - traversal.getSteps().isEmpty() ? __.identity() : traversal); - filterStep.getLabels().forEach(notStep::addLabel); - TraversalHelper.replaceStep(filterStep, notStep, parentTraversal); + if (parent instanceof ConnectiveStep) { + // wrap the child of and()/or() in not(). + final Step notStep = transformToNotStep(traversal, parent); + TraversalHelper.removeAllSteps(traversal); + traversal.addStep(notStep); + } else if (parent instanceof FilterStep) { + // converts entire where(.count().is(0)) to not(). that's why ConnectiveStep + // is handled differently above. + final Step filterStep = parent.asStep(); + final Step notStep = transformToNotStep(traversal, parent); + TraversalHelper.replaceStep(filterStep, notStep, filterStep.getTraversal()); } else { final Traversal.Admin inner; if (prev != null) { @@ -187,6 +190,19 @@ public void apply(final Traversal.Admin traversal) { } } + /** + * The {@code traversal} argument was marked as one that could be converted to {@code not()}, so wrap it inside + * that step and copy the labels. + */ + private Step transformToNotStep(final Traversal.Admin traversal, final TraversalParent parent) { + final Step filterStep = parent.asStep(); + final Traversal.Admin parentTraversal = filterStep.getTraversal(); + final Step notStep = new NotStep<>(parentTraversal, + traversal.getSteps().isEmpty() ? __.identity() : traversal.clone()); + filterStep.getLabels().forEach(notStep::addLabel); + return notStep; + } + private boolean doStrategy(final Step step) { if (!(step instanceof CountGlobalStep) || !(step.getNextStep() instanceof IsStep) || diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java index 8ea0a65eb9d..8da672e8282 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/CountStrategyTest.java @@ -70,6 +70,7 @@ public static Iterable generateTestParameters() { {__.filter(__.out().count().is(0)), __.not(__.out())}, {__.filter(__.outE().count().is(lt(1))), __.not(__.outE())}, {__.filter(__.both().count().is(lte(0))), __.not(__.both())}, + {__.filter(__.out().out().count().is(0)), __.not(__.out().out())}, {__.store("x").count().is(0).as("a"), __.store("x").limit(1).count().is(0).as("a")}, {__.out().count().as("a").is(0), __.out().limit(1).count().as("a").is(0)}, {__.out().count().is(neq(4)), __.out().limit(5).count().is(neq(4))}, @@ -90,10 +91,15 @@ public static Iterable generateTestParameters() { {__.branch(__.count().is(0)), __.branch(__.limit(1).count().is(0))}, {__.count().is(0).store("x"), __.limit(1).count().is(0).store("x")}, {__.repeat(__.out()).until(__.outE().count().is(0)), __.repeat(__.out()).until(__.not(__.outE()))}, + {__.repeat(__.out()).until(__.out().out().values("name").count().is(0)), __.repeat(__.out()).until(__.out().out().not(__.values("name")))}, + {__.repeat(__.out()).until(__.out().out().properties("age").has("x").count().is(0)), __.repeat(__.out()).until(__.out().out().not(__.properties("age").has("x")))}, {__.repeat(__.out()).emit(__.outE().count().is(0)), __.repeat(__.out()).emit(__.not(__.outE()))}, {__.where(__.outE().hasLabel("created").count().is(0)), __.not(__.outE().hasLabel("created"))}, {__.where(__.out().outE().hasLabel("created").count().is(0)), __.not(__.out().outE().hasLabel("created"))}, {__.where(__.out().outE().hasLabel("created").count().is(0).store("x")), __.where(__.out().outE().hasLabel("created").limit(1).count().is(0).store("x"))}, + {__.where(__.or(__.out("none").out().count().is(0), __.has("none"))), __.where(__.or(__.not(__.out("none").out()), __.has("none"))) }, + {__.where(__.or(__.out("none").out().count().is(0), __.has("none").count().is(0))), __.where(__.or(__.not(__.out("none").out()), __.not(__.has("none")))) }, + {__.where(__.or(__.out("none").out(), __.has("none").count().is(0))), __.where(__.or(__.out("none").out(), __.not(__.has("none")))) }, {__.filter(__.bothE().count().is(gt(0))), __.filter(__.bothE())}, {__.filter(__.bothE().count().is(gte(1))), __.filter(__.bothE())}, {__.filter(__.bothE().count().is(gt(1))), __.filter(__.bothE().limit(2).count().is(gt(1)))}, @@ -101,6 +107,7 @@ public static Iterable generateTestParameters() { {__.and(__.out().count().is(0), __.in().count().is(0)), __.and(__.not(__.out()), __.not(__.in()))}, {__.and(__.out().count().is(0), __.in().count().is(1)), __.and(__.not(__.out()), __.in().limit(2).count().is(1))}, {__.and(__.out().count().is(1), __.in().count().is(0)), __.and(__.out().limit(2).count().is(1), __.not(__.in()))}, + {__.and(__.out().out().count().is(0), __.in().count().is(0)), __.and(__.not(__.out().out()), __.not(__.in()))}, {__.or(__.out().count().is(0), __.in().count().is(0)), __.or(__.not(__.out()), __.not(__.in()))}, {__.path().filter(__.count().is(gte(0.5))).limit(5), __.path().identity().limit(5)}, // unfortunately we can't just remove the filter step {__.path().filter(__.unfold().count().is(gte(0.5))), __.path().filter(__.unfold())},