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:24 UTC
[tinkerpop] 01/01: TINKERPOP-2920 Fixed bug in SubgraphStrategy when key not present
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