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/06/16 12:43:23 UTC
[tinkerpop] branch TINKERPOP-2920 created (now fed6725a61)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2920
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
at fed6725a61 TINKERPOP-2920 Fixed bug in SubgraphStrategy when key not present
This branch includes the following new commits:
new fed6725a61 TINKERPOP-2920 Fixed bug in SubgraphStrategy when key not present
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-2920 Fixed bug in SubgraphStrategy when key not present
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2920
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit fed6725a61466e8419fce1e1c17a9608e7334735
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Thu Jun 15 15:34:52 2023 -0400
TINKERPOP-2920 Fixed bug in SubgraphStrategy when key not present
---
CHANGELOG.asciidoc | 7 ++++---
.../process/traversal/lambda/ValueTraversal.java | 7 ++++++-
.../traversal/step/map/PropertyMapStep.java | 4 ++++
.../optimization/ProductiveByStrategy.java | 20 ++++++++++++++++--
.../structure/io/graphson/GraphSONModule.java | 1 +
.../decoration/SubgraphStrategyProcessTest.java | 24 ++++++++++++++++++++++
6 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 14b1e8f724..0784fd4e1e 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -25,9 +25,10 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Fixed a memory leak in the Gremlin.Net driver that only occurred if a CancellationToken was provided.
* Added `gremlin.spark.dontDeleteNonEmptyOutput` to stop deleting the output folder if it is not empty in `spark-gremlin`.
+* Fixed a bug in `SubgraphStrategy` where the vertex property filter produced errors if a `Vertex` was missing the key provided to `by()` as a token.
* Upgraded `gremlin-javascript` and `gremlint` to Node 16.20.0.
* Upgraded `gremlin-go` to Go 1.20.
-* Improved the python Translator class
+* Improved the python `Translator` class with better handling for `P`, `None` and subclasses of `str`.
* Added `gremlin-java8.bat` file as a workaround to allow loading the console using Java 8 on Windows.
[[release-3-5-6]]
@@ -60,7 +61,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Fixed `CountStrategy` bug for cases where predicates contain negative numbers by disabling the optimization.
* Improved `count` step optimization for negative values in input for 'eq' comparison.
* Fixed performance issue when using `SampleGlobalStep` with a traverser that has either a `LABELED_PATH` or `PATH` requirement.
-* Reduce the toString() of ObjectWritable to avoid OOM for running OLAP queries on Spark.
+* Reduce the `toString()` of `ObjectWritable` to avoid OOM for running OLAP queries on Spark.
==== Bugs
@@ -138,7 +139,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Provides users with potentially more information to driver TimeoutExceptions.
* Fixed an issue in Go and Python GLVs where modifying per request settings to override request_id's was not working correctly.
* Fixed incorrect implementation for `GraphTraversalSource.With` in `gremlin-go`.
-* Changed Gremlin.version() to return `"VersionNotFound"` if the version is missing from the manifest.
+* Changed `Gremlin.version()` to return "VersionNotFound" if the version is missing from the manifest.
* Fixed local steps to avoid throwing an exception for non-iterable input.
* Fixed a case sensitivity issue when comparing request UUIDs in `gremlin-javascript`.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java
index b62927ce1f..9a8e84a5b1 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/lambda/ValueTraversal.java
@@ -115,7 +115,12 @@ public final class ValueTraversal<T, V> extends AbstractLambdaTraversal<T, V> {
"The by(\"%s\") modulator can only be applied to a traverser that is an Element or a Map - it is being applied to [%s] a %s class instead",
propertyKey, o, o.getClass().getSimpleName()));
} else {
- this.value = TraversalUtil.apply(start, this.bypassTraversal);
+ final Iterator<V> itty = TraversalUtil.applyAll(start, this.bypassTraversal);
+ if (itty.hasNext()) {
+ this.value = itty.next();
+ } else {
+ noStarts = true;
+ }
}
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java
index 12cd6becc7..16d21935e7 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java
@@ -165,6 +165,10 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>>
return propertyKeys;
}
+ public Traversal.Admin<Element, ? extends Property> getPropertyTraversal() {
+ return propertyTraversal;
+ }
+
public String toString() {
return StringFactory.stepString(this, Arrays.asList(this.propertyKeys),
this.traversalRing, this.returnType.name().toLowerCase());
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java
index ebca6017cc..29c7bee647 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java
@@ -32,6 +32,7 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.Grouping;
import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
import org.apache.tinkerpop.gremlin.process.traversal.step.map.CoalesceStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.map.ConstantStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertyMapStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
import org.apache.tinkerpop.gremlin.process.traversal.step.util.ReducingBarrierStep;
import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
@@ -92,7 +93,22 @@ public class ProductiveByStrategy extends AbstractTraversalStrategy<TraversalStr
filter(bm -> bm instanceof TraversalParent).forEach(bm -> {
final TraversalParent parentStep = (TraversalParent) bm;
final boolean parentStepIsGrouping = parentStep instanceof Grouping;
- parentStep.getLocalChildren().forEach(child -> {
+ for (Traversal.Admin child : parentStep.getLocalChildren()) {
+ // just outright skip the PropertyMapStep.propertyTraversal - it is only set by SubgraphStrategy and
+ // for this case it doesn't make sense to force that traversal to be productive. if it is forced then
+ // PropertyMap actually gets a null Property object if the traversal is not productive and that
+ // causes an NPE. It doesn't make sense to catch the NPE there really as the code wants an empty
+ // Iterator<Property> rather than a productive null. Actually, the propertyTraversal isn't even a
+ // by() provided Traversal so it really doesn't make sense for this strategy to touch it. Another
+ // sort of example where strategies shouldn't have to know so much about one another. also, another
+ // scenario where ProductiveByStrategy isn't so good. the default behavior without this strategy
+ // works so much more nicely
+ if (parentStep instanceof PropertyMapStep) {
+ final Traversal.Admin propertyTraversal = ((PropertyMapStep) parentStep).getPropertyTraversal();
+ if (propertyTraversal != null && propertyTraversal.equals(child))
+ continue;
+ }
+
// Grouping requires a bit of special handling because of TINKERPOP-2627 which is a bug that has
// some unexpected behavior for coalesce() as the value traversal. Grouping also has so inconsistencies
// in how null vs filter behavior works and that behavior needs to stay to not break in 3.5.x
@@ -120,7 +136,7 @@ public class ProductiveByStrategy extends AbstractTraversalStrategy<TraversalStr
wrapValueTraversalInCoalesce(parentStep, child);
}
}
- });
+ }
});
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONModule.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONModule.java
index eda0d37e65..cabb631449 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONModule.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphson/GraphSONModule.java
@@ -550,6 +550,7 @@ abstract class GraphSONModule extends TinkerPopJacksonModule {
MatchAlgorithmStrategy.class,
AdjacentToIncidentStrategy.class,
ByModulatorOptimizationStrategy.class,
+ ProductiveByStrategy.class,
CountStrategy.class,
FilterRankingStrategy.class,
IdentityRemovalStrategy.class,
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
index 7bb4fa9cda..a378170931 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
@@ -22,6 +22,7 @@ import org.apache.commons.configuration2.MapConfiguration;
import org.apache.tinkerpop.gremlin.LoadGraphWith;
import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest;
import org.apache.tinkerpop.gremlin.process.GremlinProcessRunner;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.optimization.ProductiveByStrategy;
import org.apache.tinkerpop.gremlin.structure.RemoteGraph;
import org.apache.tinkerpop.gremlin.process.traversal.Order;
import org.apache.tinkerpop.gremlin.process.traversal.P;
@@ -38,6 +39,8 @@ import org.apache.tinkerpop.gremlin.structure.Column;
import org.apache.tinkerpop.gremlin.structure.Edge;
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator;
+import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyVertexProperty;
+import org.apache.tinkerpop.gremlin.structure.util.reference.ReferenceVertexProperty;
import org.hamcrest.Matchers;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -45,12 +48,14 @@ import org.junit.runner.RunWith;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
+import java.util.Map;
import java.util.NoSuchElementException;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.CREW;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.both;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.bothE;
+import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.constant;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.hasNot;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.out;
@@ -59,6 +64,7 @@ import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsCollectionContaining.hasItems;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeThat;
@@ -508,6 +514,24 @@ public class SubgraphStrategyProcessTest extends AbstractGremlinProcessTest {
checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().outE("uses").values("skill"));
checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().values("skill"));
checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().properties("skill").as("b").identity().select("b").by(__.value()));
+ //
+
+ // testing situations where the key specified is not present for the vertices - TINKERPOP-2920 use of
+ // constant(true) here is sorta weird because a Traversal<?,VertexProperty> is expected, but not really
+ sg = g.withStrategies(SubgraphStrategy.create(new MapConfiguration(new HashMap<String, Object>() {{
+ put(SubgraphStrategy.VERTEX_PROPERTIES, constant(true));
+ }})));
+ final List<Map<String, Object>> l = sg.V().project("example").by("example").toList();
+ l.forEach(map -> {
+ assertEquals(1, map.size());
+ assertNull(map.get("example"));
+ });
+ checkResults(Arrays.asList("aachen","baltimore","bremen","brussels","centreville","dulles","kaiserslautern",
+ "oakland","purcellville","san diego","santa cruz","santa fe","seattle","spremberg"),
+ sg.withoutStrategies(ProductiveByStrategy.class).V().valueMap("location").select(Column.values).unfold().unfold());
+ checkResults(Arrays.asList("aachen","baltimore","bremen","brussels","centreville","dulles","kaiserslautern",
+ "oakland","purcellville","san diego","santa cruz","santa fe","seattle","spremberg"),
+ sg.V().valueMap("location").select(Column.values).unfold().unfold());
}
@Test