This is an automated email from the ASF dual-hosted git repository. kenhuuu pushed a commit to branch TINKERPOP-2974 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 62d415673b4c27ab538d27d02036eef347fc1536 Author: Ken Hu <[email protected]> AuthorDate: Mon Mar 24 14:31:08 2025 -0700 TINKERPOP-2974 Change PropertyMap to only allow single by() if order not guaranteed Not all implementations of Gremlin are going to be able to iterate the map produced by PropertyMap step in order. This means that multiple by()s can't be applied in order. Therefore, for those implementations, an exception should be thrown instead. --- docs/src/dev/provider/gremlin-semantics.asciidoc | 39 ++++++++++++++++++++++ docs/src/upgrade/release-3.8.x.asciidoc | 22 ++++++++++++ .../traversal/step/map/PropertyMapStep.java | 37 ++++++++++---------- .../tinkerpop/gremlin/features/StepDefinition.java | 4 +++ .../gremlin/test/features/map/ValueMap.feature | 19 ++--------- .../structure/TinkerShuffleGraphTest.java | 15 +++++++++ 6 files changed, 101 insertions(+), 35 deletions(-) diff --git a/docs/src/dev/provider/gremlin-semantics.asciidoc b/docs/src/dev/provider/gremlin-semantics.asciidoc index d16f0a9972..37610d9d5d 100644 --- a/docs/src/dev/provider/gremlin-semantics.asciidoc +++ b/docs/src/dev/provider/gremlin-semantics.asciidoc @@ -1615,3 +1615,42 @@ Null values from the incoming traverser are not processed and remain as null whe See: link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TrimGlobalStep.java[source], link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TrimLocalStep.java[source (local)], link:https://tinkerpop.apache.org/docs/x.y.z/reference/#trim-step[reference] + +[[valueMap-step]] +=== valueMap() + +*Description:* Removes repeatedly seen results from the Traversal Stream. + +*Syntax:* `valueMap(String... propertyKeys)` | `valueMap(boolean includeTokens, String... propertyKeys)` + +[width="100%",options="header"] +|========================================================= +|Start Step |Mid Step |Modulated |Domain |Range +|N |Y |`by()`/`with()` |`Element` |`Map` +|========================================================= + +*Arguments:* + +* `includeTokens` - Determines if the result map should contain entries from `T` (such as `label` and `id`). + +* `propertyKeys` - If `valueMap()` is provided a list of propertyKeys, then the result map will only contain entries +specified by propertyKeys. If the list is empty, then all properties are included. + +*Modulation:* + +* `by(Traversal)` - Semantics depend on whether the implementation is capable of iterating the result map in order. +** If iteration order guaranteed - applies the by() traversals in a round-robin fashion to the map values. +** If iteration order randomized - allows only a single by() traversal and applies it to all map values. An attempt to +add more than one by() will result in an Exception that contains the message "valueMap() step only accepts one by() +modulator which has already been set". +* `with(key, value)` - Determines which optional entries to add to the map. The key is "tinkerpop.valueMap.tokens" and +the possible values are: +** 0 for no tokens +** 1 for including id (Element) +** 2 for including label (Vertex/Edge) +** 4 for including keys (VertexProperty) +** 8 for including values (VertexProperty) +** 15 for including all + +See: link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java[source], +link:https://tinkerpop.apache.org/docs/x.y.z/reference/#valuemap-step[reference] diff --git a/docs/src/upgrade/release-3.8.x.asciidoc b/docs/src/upgrade/release-3.8.x.asciidoc index 02110a836e..1d36b648bc 100644 --- a/docs/src/upgrade/release-3.8.x.asciidoc +++ b/docs/src/upgrade/release-3.8.x.asciidoc @@ -177,4 +177,26 @@ skipped with incorrect result. ==== Graph System Providers +===== Semantics change for valueMap() + +The semantics have changed for the handling of by modulators to the `valueMap()` step. Providers will now have two +different options. If map entries are iterated in the same order as the keys given argument list, then nothing has +changed and the by modulators should be applied in a round robin fashion. If the map entries aren't iterated in, then +only one by modulator is required to be accepted and an exception should be thrown when there are more than one by() +modulators. The exception thrown should contain the following: "valueMap() step only accepts one by() modulator which +has already been set" + +The reference implementation of `valueMap()` in `gremlin-core` has been updated to handle this. In particular, it makes +use of a new Graph Feature to determine what should be done. See the next section for more information. + +See: link:https://issues.apache.org/jira/browse/TINKERPOP-2974[TINKERPOP-2974] + +===== New Graph Features for ordering + +`supportsOrderedProperties()` has been added to `ElementFeatures` to allow providers to advertise whether or not they +are able to return properties in the order that they are specified in +`Iterator<? extends Property<V>> properties(final String... propertyKeys)`. If the `Iterator` returned by +`properties(String...)` will yield results in the exact order provided by the `String...` argument then the provider +should return `true`. + ==== Graph Driver Providers 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 812f13b4d2..8426c3fd6f 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 @@ -27,7 +27,6 @@ import org.apache.tinkerpop.gremlin.process.traversal.step.util.Parameters; import org.apache.tinkerpop.gremlin.process.traversal.step.util.WithOptions; import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalProduct; -import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalRing; import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil; import org.apache.tinkerpop.gremlin.structure.Element; import org.apache.tinkerpop.gremlin.structure.Property; @@ -61,18 +60,14 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> protected Traversal.Admin<Element, ? extends Property> propertyTraversal; protected Parameters parameters = new Parameters(); - protected TraversalRing<K, E> traversalRing; + protected Traversal.Admin<K, E> valueTraversal; - public PropertyMapStep(final Traversal.Admin traversal, final PropertyType propertyType, TraversalRing<K, E> traversalRing, final String... propertyKeys) { + public PropertyMapStep(final Traversal.Admin traversal, final PropertyType propertyType, final String... propertyKeys) { super(traversal); this.propertyKeys = propertyKeys; this.returnType = propertyType; this.propertyTraversal = null; - this.traversalRing = traversalRing; - } - - public PropertyMapStep(final Traversal.Admin traversal, final PropertyType propertyType, final String... propertyKeys) { - this(traversal, propertyType, new TraversalRing<>(), propertyKeys); + this.valueTraversal = null; } public PropertyMapStep(final Traversal.Admin traversal, final int options, final PropertyType propertyType, final String... propertyKeys) { @@ -117,13 +112,17 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> final List<Traversal.Admin<K, E>> result = new ArrayList<>(); if (null != this.propertyTraversal) result.add((Traversal.Admin) propertyTraversal); - result.addAll(this.traversalRing.getTraversals()); + result.add(this.valueTraversal); return Collections.unmodifiableList(result); } @Override public void modulateBy(final Traversal.Admin<?, ?> selectTraversal) { - this.traversalRing.addTraversal(this.integrateChild(selectTraversal)); + if (null == valueTraversal) { + this.valueTraversal = this.integrateChild(selectTraversal); + } else { + throw new IllegalArgumentException("propertyMap() step can only have one by modulator"); + } } public void setPropertyTraversal(final Traversal.Admin<Element, ? extends Property> propertyTraversal) { @@ -144,7 +143,7 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> public String toString() { return StringFactory.stepString(this, Arrays.asList(this.propertyKeys), - this.traversalRing, this.returnType.name().toLowerCase()); + this.valueTraversal, this.returnType.name().toLowerCase()); } @Override @@ -152,7 +151,7 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> final PropertyMapStep<K,E> clone = (PropertyMapStep<K,E>) super.clone(); if (null != this.propertyTraversal) clone.propertyTraversal = this.propertyTraversal.clone(); - clone.traversalRing = this.traversalRing.clone(); + clone.valueTraversal = this.valueTraversal.clone(); return clone; } @@ -164,7 +163,7 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> for (final String propertyKey : this.propertyKeys) { result ^= Objects.hashCode(propertyKey); } - return result ^ this.traversalRing.hashCode(); + return result ^ this.valueTraversal.hashCode(); } @Override @@ -172,7 +171,8 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> super.setTraversal(parentTraversal); if (null != this.propertyTraversal) this.integrateChild(this.propertyTraversal); - this.traversalRing.getTraversals().forEach(this::integrateChild); + if (null != this.valueTraversal) + integrateChild(this.valueTraversal); } @Override @@ -241,22 +241,21 @@ public class PropertyMapStep<K,E> extends ScalarMapStep<Element, Map<K, E>> } protected void applyTraversalRingToMap(Map<Object, Object> map){ - if (!traversalRing.isEmpty()) { + if (this.valueTraversal != null) { // 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()); + final TraversalProduct product = TraversalUtil.produce(v, (Traversal.Admin) this.valueTraversal); // compute() should take the null and remove the key return product.isProductive() ? product.get() : null; }); } - this.traversalRing.reset(); } } - public TraversalRing<K, E> getTraversalRing() { - return traversalRing; + public Traversal.Admin<K, E> getValueTraversal() { + return this.valueTraversal; } } diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/features/StepDefinition.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/features/StepDefinition.java index 13c6debeac..7aa3e2cd80 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/features/StepDefinition.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/features/StepDefinition.java @@ -307,6 +307,10 @@ public final class StepDefinition { @When("iterated to list") public void iteratedToList() { + if (null == traversal) { + return; // Error must have occurred during traversal construction. Skip to prevent NPE. + } + try { result = traversal.toList(); } catch (Exception ex) { diff --git a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/ValueMap.feature b/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/ValueMap.feature index 65e495ffca..25508d6f78 100644 --- a/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/ValueMap.feature +++ b/gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/ValueMap.feature @@ -184,9 +184,7 @@ Feature: Step - valueMap() | m[{"name": ["josh"], "age": [32], "t[label]":"person", "t[id]":"v[josh].id"}] | | m[{"name": ["peter"], "age": [35], "t[label]":"person", "t[id]":"v[peter].id"}] | -# NOTE: Insertion order is required for this test due to TINKERPOP-2974. -# This annotation should be removed once the bug is fixed - @MultiProperties @MetaProperties @InsertionOrderingRequired + @MultiProperties @MetaProperties Scenario: g_VX1X_valueMapXname_locationX_byXunfoldX_by Given the crew graph And using the parameter vid1 defined as "v[marko].id" @@ -195,9 +193,7 @@ Feature: Step - valueMap() g.V(vid1).valueMap("name", "location").by(__.unfold()).by() """ When iterated to list - Then the result should be unordered - | result | - | m[{"name": "marko", "location": ["san diego", "santa cruz", "brussels", "santa fe"]}] | + Then the traversal will raise an error with message containing text of "step can only have one by modulator" Scenario: g_V_valueMapXname_age_nullX Given the modern graph @@ -215,8 +211,6 @@ Feature: Step - valueMap() | m[{"name": ["lop"]}] | | m[{"name": ["ripple"]}] | -# NOTE: Insertion order is required for this test due to TINKERPOP-2974. -# This annotation should be removed once the bug is fixed @InsertionOrderingRequired Scenario: g_V_valueMapXname_ageX_byXisXxXXbyXunfoldX Given the modern graph @@ -225,11 +219,4 @@ Feature: Step - valueMap() g.V().valueMap("name", "age").by(__.is("x")).by(__.unfold()) """ When iterated to list - Then the result should be unordered - | result | - | m[{"age": 29}] | - | m[{"age": 32}] | - | m[{"age": 35}] | - | m[{"age": 27}] | - | m[{}] | - | m[{}] | \ No newline at end of file + Then the traversal will raise an error with message containing text of "step can only have one by modulator" diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerShuffleGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerShuffleGraphTest.java index 577b824898..dfcf67db4e 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerShuffleGraphTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerShuffleGraphTest.java @@ -19,6 +19,7 @@ package org.apache.tinkerpop.gremlin.tinkergraph.structure; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.structure.Edge; import org.apache.tinkerpop.gremlin.structure.Property; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -37,6 +38,9 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.not; import static org.hamcrest.collection.IsIterableContainingInAnyOrder.containsInAnyOrder; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author Cole Greer (https://github.com/Cole-Greer) @@ -180,6 +184,17 @@ public class TinkerShuffleGraphTest { assertThat(resultRegular, not(contains(resultShuffle))); } + @Test + public void shouldThrowForPropertyMapTraversalWithMultipleBy() { + final GraphTraversalSource gShuff = createShuffleModern(); + try { + gShuff.V().valueMap("name", "age").by().by(__.unfold()); + fail("Should have thrown exception"); + } catch (Exception e) { + assertTrue(e.getMessage().contains("")); + } + } + private GraphTraversalSource createShuffleModern() { TinkerShuffleGraph graph = TinkerShuffleGraph.open(); TinkerFactory.generateModern(graph);
