You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2023/04/11 12:55:45 UTC
[tinkerpop] branch TINKERPOP-2911 created (now 39e3a74296)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2911
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
at 39e3a74296 TINKERPOP-2911 Fixed bug in CountStrategy around ConnectiveStep
This branch includes the following new commits:
new 39e3a74296 TINKERPOP-2911 Fixed bug in CountStrategy around ConnectiveStep
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[tinkerpop] 01/01: TINKERPOP-2911 Fixed bug in CountStrategy around ConnectiveStep
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2911
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 39e3a74296ed13d0011d50f0b0234061d8808af9
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Tue Apr 11 08:53:50 2023 -0400
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 ++++++++++++++++------
.../strategy/optimization/CountStrategyTest.java | 7 +++++
3 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index fc18243d55..e36e9ccf34 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 3def969147..d4cc525641 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 final class CountStrategy extends AbstractTraversalStrategy<TraversalStra
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(<x>.count().is(0)) to not(<x>). 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 final class CountStrategy extends AbstractTraversalStrategy<TraversalStra
}
}
+ /**
+ * 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 8ea0a65eb9..8da672e828 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 class CountStrategyTest {
{__.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 class CountStrategyTest {
{__.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 class CountStrategyTest {
{__.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())},