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/07/07 17:39:18 UTC
[tinkerpop] 02/03: Merge branch '3.5-dev' into 3.6-dev
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 82a78fcf744ad4dc971ab7817ffd233fc00206fb
Merge: b355b6d002 b2fb2b2465
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Fri Jul 7 13:37:56 2023 -0400
Merge branch '3.5-dev' into 3.6-dev
CHANGELOG.asciidoc | 5 +++--
.../process/traversal/lambda/ValueTraversal.java | 7 ++++++-
.../traversal/step/map/PropertyMapStep.java | 8 ++++----
.../optimization/ProductiveByStrategy.java | 24 +++++++++++++++++-----
.../structure/io/graphson/GraphSONModule.java | 1 +
.../decoration/SubgraphStrategyProcessTest.java | 23 +++++++++++++++++++++
6 files changed, 56 insertions(+), 12 deletions(-)
diff --cc CHANGELOG.asciidoc
index 58ec3f2452,07f4ecb222..1a0eb72529
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@@ -359,12 -23,13 +359,13 @@@ image::https://raw.githubusercontent.co
[[release-3-5-7]]
=== TinkerPop 3.5.7 (Release Date: NOT OFFICIALLY RELEASED YET)
-* Fixed a memory leak in the Gremlin.Net driver that only occurred if a CancellationToken was provided.
+* Fixed a memory leak in the Gremlin.Net driver that only occurred if a `CancellationToken` was provided.
* Fixed gremlin-python `Client` problem where calling `submit()` after` `close()` would hang the system.
* 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.
* Fixed a bug in `gremlin-server` where timeout tasks were not cancelled and could cause very large memory usage when timeout is large.
* Removed `jcabi-manifests` dependency from `gremlin-core`, `gremlin-driver`, and `gremlin-server`.
diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java
index f089636572,16d21935e7..812f13b4d2
--- 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
@@@ -180,83 -211,7 +184,79 @@@ public class PropertyMapStep<K,E> exten
return this.tokens;
}
- private boolean includeToken(final int token) {
+ protected boolean includeToken(final int token) {
return 0 != (this.tokens & token);
}
+
+ protected void addIncludedOptions(Element element, Map<Object, Object> map){
+ if (this.returnType == PropertyType.VALUE) {
+ if (includeToken(WithOptions.ids)) map.put(T.id, getElementId(element));
+ if (element instanceof VertexProperty) {
+
+ if (includeToken(WithOptions.keys)) map.put(T.key, getVertexPropertyKey((VertexProperty<?>) element));
+ if (includeToken(WithOptions.values)) map.put(T.value, getVertexPropertyValue((VertexProperty<?>) element));
+ } else {
+ if (includeToken(WithOptions.labels)) map.put(T.label, getElementLabel(element));
+ }
+ }
+ }
+
+ protected Object getElementId(Element element){
+ return element.id();
+ }
+
+ protected String getElementLabel(Element element){
+ return element.label();
+ }
+
+ protected String getVertexPropertyKey(VertexProperty<?> vertexProperty){
+ return vertexProperty.key();
+ }
+
+ protected Object getVertexPropertyValue(VertexProperty<?> vertexProperty){
+ return vertexProperty.value();
+ }
+
+ protected void addElementProperties(final Traverser.Admin<Element> traverser, Map<Object, Object> map){
+ final Element element = traverser.get();
+ final boolean isVertex = element instanceof Vertex;
+
+ final Iterator<? extends Property> properties = null == this.propertyTraversal ?
+ element.properties(this.propertyKeys) :
+ TraversalUtil.applyAll(traverser, this.propertyTraversal);
+
+ while (properties.hasNext()) {
+ final Property<?> property = properties.next();
+ final Object value = this.returnType == PropertyType.VALUE ? property.value() : property;
+ if (isVertex) {
+ map.compute(property.key(), (k, v) -> {
+ final List<Object> values = v != null ? (List<Object>) v : new ArrayList<>();
+ values.add(value);
+ return values;
+ });
+ } else {
+ map.put(property.key(), value);
+ }
+ }
+ }
+
+ protected void applyTraversalRingToMap(Map<Object, Object> map){
+ if (!traversalRing.isEmpty()) {
+ // will cop a ConcurrentModification if a key is dropped so need this little copy here
+ final List<Object> keys = new ArrayList<>(map.keySet());
+ for (final Object key : keys) {
+ map.compute(key, (k, v) -> {
+ final TraversalProduct product = TraversalUtil.produce(v, (Traversal.Admin) this.traversalRing.next());
+
+ // compute() should take the null and remove the key
+ return product.isProductive() ? product.get() : null;
+ });
+ }
+ this.traversalRing.reset();
+ }
+ }
+
- public Traversal.Admin<Element, ? extends Property> getPropertyTraversal() {
- return propertyTraversal;
- }
-
+ public TraversalRing<K, E> getTraversalRing() {
+ return traversalRing;
+ }
}
diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java
index ba7b92d3c3,29c7bee647..ccdb820bce
--- 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
@@@ -21,14 -21,14 +21,12 @@@ package org.apache.tinkerpop.gremlin.pr
import org.apache.commons.configuration2.Configuration;
import org.apache.commons.configuration2.MapConfiguration;
--import org.apache.tinkerpop.gremlin.process.traversal.Step;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal;
import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal;
import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating;
--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;
@@@ -91,26 -92,51 +90,41 @@@ public class ProductiveByStrategy exten
TraversalHelper.getStepsOfAssignableClass(ByModulating.class, traversal).stream().
filter(bm -> bm instanceof TraversalParent).forEach(bm -> {
final TraversalParent parentStep = (TraversalParent) bm;
- parentStep.getLocalChildren().forEach(child -> {
- if (child instanceof ValueTraversal && !containsValidByPass((ValueTraversal) child) &&
- final boolean parentStepIsGrouping = parentStep instanceof Grouping;
+ 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
++ // 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
- if (!parentStepIsGrouping) {
- if (child instanceof ValueTraversal && hasKeyNotKnownAsProductive((ValueTraversal) child)) {
- wrapValueTraversalInCoalesce(parentStep, child);
- } else if (!(child.getEndStep() instanceof ReducingBarrierStep)) {
- // ending reducing barrier will always return something so seems safe to not bother wrapping
- // that up in coalesce().
- final Traversal.Admin extractedChildTraversal = new DefaultGraphTraversal<>();
- TraversalHelper.removeToTraversal(child.getStartStep(), EmptyStep.instance(), extractedChildTraversal);
- child.addStep(new CoalesceStep(child, extractedChildTraversal, nullTraversal));
++ if (child instanceof ValueTraversal && !containsValidByPass((ValueTraversal) child) &&
+ hasKeyNotKnownAsProductive((ValueTraversal) child)) {
+ wrapValueTraversalInCoalesce(parentStep, child);
+ } else if (!(child.getEndStep() instanceof ReducingBarrierStep)) {
+ // ending reducing barrier will always return something so seems safe to not bother wrapping
+ // that up in coalesce().
+ final Traversal.Admin extractedChildTraversal = new DefaultGraphTraversal<>();
+ TraversalHelper.removeToTraversal(child.getStartStep(), EmptyStep.instance(), extractedChildTraversal);
+ child.addStep(new CoalesceStep(child, extractedChildTraversal, nullTraversal));
- // replace so that internally the parent step gets to re-initialize the child as it may need to.
- try {
- parentStep.replaceLocalChild(child, child);
- } catch (IllegalStateException ignored) {
- // ignore situations where the parent traversal doesn't support replacement. in those cases
- // we simply retain whatever the original behavior was even if it is inconsistent
- }
- }
- } else {
- if (child instanceof ValueTraversal && ((Grouping) parentStep).getKeyTraversal() == child &&
- hasKeyNotKnownAsProductive((ValueTraversal) child) && !containsValidByPass((ValueTraversal) child)) {
- wrapValueTraversalInCoalesce(parentStep, child);
+ // replace so that internally the parent step gets to re-initialize the child as it may need to.
+ try {
+ parentStep.replaceLocalChild(child, child);
+ } catch (IllegalStateException ignored) {
+ // ignore situations where the parent traversal doesn't support replacement. in those cases
+ // we simply retain whatever the original behavior was even if it is inconsistent
}
}
- });
+ }
});
}
diff --cc gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java
index 7bb4fa9cda,a378170931..ef26a080d2
--- 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
@@@ -508,6 -514,24 +514,23 @@@ public class SubgraphStrategyProcessTes
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"));
++ assertEquals(0, map.size());
+ });
+ 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