This is an automated email from the ASF dual-hosted git repository.
colegreer pushed a commit to branch 3.8-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.8-dev by this push:
new b88cba81b2 TINKERPOP-2974 Change PropertyMap to only allow single by()
if order not guaranteed (#3098)
b88cba81b2 is described below
commit b88cba81b2ae6aaea580920f543a4590d76009da
Author: kenhuuu <[email protected]>
AuthorDate: Tue Apr 29 11:58:20 2025 -0700
TINKERPOP-2974 Change PropertyMap to only allow single by() if order not
guaranteed (#3098)
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.
---
CHANGELOG.asciidoc | 1 +
docs/src/dev/provider/gremlin-semantics.asciidoc | 38 ++++++++++++++++++++
docs/src/upgrade/release-3.8.x.asciidoc | 15 ++++++++
.../traversal/step/map/PropertyMapStep.java | 41 ++++++++++++----------
.../process/traversal/step/map/ValueMapTest.java | 22 ++++++------
.../gremlin/test/features/map/ValueMap.feature | 21 +++--------
6 files changed, 90 insertions(+), 48 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index b1c5577710..678c8a15d2 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -65,6 +65,7 @@ This release also includes changes from <<release-3-7-XXX,
3.7.XXX>>.
* Added and made `OffsetDateTime` serializers the default for existing date
types in Python, Go, JavaScript, and .NET. `Date` is only used to deserialize
from server.
* Added missing strategies in `gremlin-go`, updated certain strategies to take
varargs and updated `GoTranslatorVisitor` for corresponding translations.
* Fixed `BigInt` and `BigDecimal` parsing in `gremlin-go` cucumber test suite,
fixed `UnscaledValue` type in `BigDecimal` struct and added `ParseBigDecimal`
method.
+* Added validation to `valueMap()`/`propertyMap()`/`groupCount()` to prevent
an invalid usage of multiple `by()` modulators.
* Added validation to `groupCount()` to prevent an invalid usage of multiple
`by()` modulators.
* Deprecated `ProcessLimitedStandardSuite` and `ProcessLimitedComputerSuite`
in favor of `ProcessEmbeddedStandardSuite` and `ProcessEmbeddedComputerSuite`
respectively.
* Deprecated `ProcessStandardSuite` and the `ProcessComputerSuite` in favor of
Gherkin testing and the `ProcessEmbeddedStandardSuite` and
`ProcessEmbeddedComputerSuite` for testing JVM-specific Gremlin behaviors.
diff --git a/docs/src/dev/provider/gremlin-semantics.asciidoc
b/docs/src/dev/provider/gremlin-semantics.asciidoc
index d16f0a9972..f0eca9a725 100644
--- a/docs/src/dev/provider/gremlin-semantics.asciidoc
+++ b/docs/src/dev/provider/gremlin-semantics.asciidoc
@@ -1615,3 +1615,41 @@ 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:* Converts elements to a map representation of their properties.
+
+*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)` - Only a single by() modulator is allowed and it applies to
all map values. An attempt to add more
+than one by() will result in an Exception that contains the message
"valueMap()/propertyMap() step can only have one by
+modulator".
+* `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],
+link:https://tinkerpop.apache.org/docs/x.y.z/reference/#propertymap-step[reference]
diff --git a/docs/src/upgrade/release-3.8.x.asciidoc
b/docs/src/upgrade/release-3.8.x.asciidoc
index 97e9359932..8fa1df39cc 100644
--- a/docs/src/upgrade/release-3.8.x.asciidoc
+++ b/docs/src/upgrade/release-3.8.x.asciidoc
@@ -296,6 +296,13 @@ gremlin>
g.V().has("person","name",P.within("vadas","peter")).group().by().by(__
See: link:https://issues.apache.org/jira/browse/TINKERPOP-2971[TINKERPOP-2971]
+==== valueMap() and propertyMap() By Modulation Semantics
+
+The `valueMap()` and `propertyMap()`steps have been changed to throw an error
if multiple `by()` modulators are applied.
+The previous behavior attempted to round-robin the by() but this wasn't
possible for all providers.
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2974[TINKERPOP-2974]
+
==== groupCount() By Modulation Semantics
The `groupCount()` step has been changed to throw an error if multiple `by()`
modulators are applied. The previous
@@ -332,6 +339,14 @@ The default implementation for date type in Gremlin is now
changed from the depr
This may impact providers who use TinkerGraph or whose implementation store
dates as `java.util.Date`. While steps will support `Date`, all date
manipulations will output `OffsetDateTime`. If a user had persisted `Date`
objects in the database, upgrading to 3.8 may lead to the database having both
types stored. It is recommended for users to perform transformation of `Date`
to `OffsetDateTime` to retain consistency.
+===== Semantics change for PropertyMapStep
+
+The semantics have changed for the handling of by modulators to the `valueMap`
and `propertyMap` steps. 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()/propertyMap()
step can only have one by modulator".
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2974[TINKERPOP-2974]
+
==== Graph Driver Providers
==== The Switch from Date to OffsetDateTime
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..3534b7e313 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,18 @@ 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());
+ if (null != this.valueTraversal)
+ 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("valueMap()/propertyMap() step
can only have one by modulator");
+ }
}
public void setPropertyTraversal(final Traversal.Admin<Element, ? extends
Property> propertyTraversal) {
@@ -144,7 +144,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 +152,8 @@ 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();
+ if (null != this.valueTraversal)
+ clone.valueTraversal = this.valueTraversal.clone();
return clone;
}
@@ -161,10 +162,12 @@ public class PropertyMapStep<K,E> extends
ScalarMapStep<Element, Map<K, E>>
int result = super.hashCode() ^ this.returnType.hashCode() ^
Integer.hashCode(this.tokens);
if (null != this.propertyTraversal)
result ^= this.propertyTraversal.hashCode();
+ if (null != this.valueTraversal)
+ result ^= this.valueTraversal.hashCode();
for (final String propertyKey : this.propertyKeys) {
result ^= Objects.hashCode(propertyKey);
}
- return result ^ this.traversalRing.hashCode();
+ return result;
}
@Override
@@ -172,7 +175,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 +245,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/process/traversal/step/map/ValueMapTest.java
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java
index dac12d4131..f54df0c5ad 100644
---
a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java
+++
b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java
@@ -35,15 +35,18 @@ import java.util.Map;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.CREW;
import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
+import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.apache.tinkerpop.gremlin.structure.T.id;
import static org.apache.tinkerpop.gremlin.structure.T.label;
+import static org.hamcrest.core.StringEndsWith.endsWith;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
@@ -345,19 +348,14 @@ public abstract class ValueMapTest extends
AbstractGremlinProcessTest {
@Test
@LoadGraphWith(CREW)
public void g_VX1X_valueMapXname_locationX_byXunfoldX_by() {
- final Traversal<Vertex,Map<Object,Object>> traversal =
get_g_VX1X_valueMapXname_locationX_byXunfoldX_by(convertToVertexId("marko"));
- printTraversalForm(traversal);
- assertTrue(traversal.hasNext());
- final Map<Object, Object> values = traversal.next();
- assertFalse(traversal.hasNext());
- assertTrue("name value should be a String", values.containsKey("name")
&& String.class.equals(values.get("name").getClass()));
- assertTrue("location value should be a List",
values.containsKey("location") &&
List.class.isAssignableFrom(values.get("location").getClass()));
- assertEquals("marko", values.get("name"));
- final List locations = (List) values.get("location");
- for (final String location : Arrays.asList("san diego", "santa cruz",
"brussels", "santa fe")) {
- assertTrue(locations.contains(location));
+ try {
+ final Traversal<Vertex,Map<Object,Object>> traversal =
get_g_VX1X_valueMapXname_locationX_byXunfoldX_by(convertToVertexId("marko"));
+ printTraversalForm(traversal);
+ assertFalse(traversal.hasNext());
+ fail("Should have failed as multiple by() not allowed for
valueMap");
+ } catch (Exception ex) {
+ assertThat(ex.getMessage(),
containsString("valueMap()/propertyMap() step can only have one by modulator"));
}
- assertEquals(2, values.size());
}
public static class Traversals extends ValueMapTest {
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..6427aaf061 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"
@@ -194,10 +192,8 @@ 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"]}] |
+ When iterated next
+ 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"
