You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "Cole-Greer (via GitHub)" <gi...@apache.org> on 2025/09/22 22:50:06 UTC
[PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Cole-Greer opened a new pull request, #3215:
URL: https://github.com/apache/tinkerpop/pull/3215
https://issues.apache.org/jira/browse/TINKERPOP-3193
Updates AddVertexStep, AddVertexStartStep, AddEdgeStep, AddEdgeStartStep, and AddPropertyStep
to isolate internal state management (used for id, label, properties, from/to...) from the
Paramenters object used by Configuring and Parameters interfaces. These interfaces are now
exclusively used for the purposes of with() modulation. All other existing usages of Parameters
in Configuring steps is exclusively related to with() modulation.
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380015583
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -888,6 +874,41 @@ pinned variable is one which must remain bound to the current value. By paramete
variable, as it can be substituted in the optimized traversal without any loss of meaning, however the variable "count"
will be pinned as the optimized traversal is not valid for other values of "count".
+===== Decouple internal step state from `Configuring`/`Parameterizing` interfaces
+
+Previously `AddVertexStep`, `AddVertexStartStep`, `AddEdgeStep`, `AddEdgeStartStep`, and `AddPropertyStep` stored their
+internal state (id, label, properties, from/to vertices...) in a `Parameters` object which was exposed via the
+`Configuring` and `Parameterizing` interfaces. These interfaces are primarily intended to support with()-modulation, and
+thus these steps had weird unintended behaviors when combined with with()-modulation:
+
+[source,text]
+----
+gremlin> g.addV().with("name", "cole").valueMap()
+==>[name:[cole]]
Review Comment:
What is this example intended to inform? Is it just an example of usage of `with`? I'm guessing it's supposed to demonstrate how it's odd to be able to set properties using `with()`? Regardless please specify what is unexpected about this example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#issuecomment-3340548126
VOTE +1, pending fixes for others' comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380252135
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java:
##########
@@ -60,6 +61,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends AbstractStep<
*/
private Map<Object, List<Object>> properties = new HashMap<>();
+ private Parameters withConfiguration = new Parameters();
Review Comment:
Should modify hashCode to include this new field
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2389282986
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java:
##########
@@ -60,6 +61,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends AbstractStep<
*/
private Map<Object, List<Object>> properties = new HashMap<>();
+ private Parameters withConfiguration = new Parameters();
Review Comment:
Yes to traversals, no to GValues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380237199
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -202,42 +208,43 @@ public VertexProperty.Cardinality getCardinality() {
@Override
public Object getKey() {
- List<Object> keyParams = parameters.get(T.key, null);
+ List<Object> keyParams = internalParameters.get(T.key, null);
return keyParams.isEmpty() ? null : keyParams.get(0);
}
@Override
public Object getValue() {
- List<Object> values = parameters.get(T.value, null);
+ List<Object> values = internalParameters.get(T.value, null);
return values.isEmpty() ? null : values.get(0);
}
@Override
public String toString() {
- return StringFactory.stepString(this, this.parameters);
+ return StringFactory.stepString(this, this.internalParameters);
}
@Override
public AddPropertyStep<S> clone() {
final AddPropertyStep<S> clone = (AddPropertyStep<S>) super.clone();
- clone.parameters = this.parameters.clone();
+ clone.internalParameters = this.internalParameters.clone();
+ clone.withConfiguration = this.withConfiguration.clone();
return clone;
}
@Override
public void addProperty(Object key, Object value) {
- configure(key, value);
+ configureInternalParams(key, value);
Review Comment:
Nit:
```suggestion
this.internalParameters.set(this, key, value);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2382927510
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -888,6 +874,41 @@ pinned variable is one which must remain bound to the current value. By paramete
variable, as it can be substituted in the optimized traversal without any loss of meaning, however the variable "count"
will be pinned as the optimized traversal is not valid for other values of "count".
+===== Decouple internal step state from `Configuring`/`Parameterizing` interfaces
+
+Previously `AddVertexStep`, `AddVertexStartStep`, `AddEdgeStep`, `AddEdgeStartStep`, and `AddPropertyStep` stored their
+internal state (id, label, properties, from/to vertices...) in a `Parameters` object which was exposed via the
+`Configuring` and `Parameterizing` interfaces. These interfaces are primarily intended to support with()-modulation, and
+thus these steps had weird unintended behaviors when combined with with()-modulation:
+
+[source,text]
+----
+gremlin> g.addV().with("name", "cole").valueMap()
+==>[name:[cole]]
+----
+
+This internal step state has now been decoupled from the `Configuring` and `Parameterizing` interfaces, which are now
+exclusively used for the purposes of with()-modulation. All accesses of these steps internal data must now go through
Review Comment:
I will update with an explicit list of steps. It's primarily a concern for strategy authors and providers as this is changing how data such as properties is accessed from steps such as addV. The only users who will be impacted are those doing silly things like using `with()` to set properties, which has never been a documented or supported behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2382920510
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -888,6 +874,41 @@ pinned variable is one which must remain bound to the current value. By paramete
variable, as it can be substituted in the optimized traversal without any loss of meaning, however the variable "count"
will be pinned as the optimized traversal is not valid for other values of "count".
+===== Decouple internal step state from `Configuring`/`Parameterizing` interfaces
+
+Previously `AddVertexStep`, `AddVertexStartStep`, `AddEdgeStep`, `AddEdgeStartStep`, and `AddPropertyStep` stored their
+internal state (id, label, properties, from/to vertices...) in a `Parameters` object which was exposed via the
+`Configuring` and `Parameterizing` interfaces. These interfaces are primarily intended to support with()-modulation, and
+thus these steps had weird unintended behaviors when combined with with()-modulation:
+
+[source,text]
+----
+gremlin> g.addV().with("name", "cole").valueMap()
+==>[name:[cole]]
Review Comment:
Yes it's just demonstrating the unintended consequence of the previous overloaded usage of `Configuring`. I will add more context to the example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380240055
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -48,41 +48,46 @@
* @author Marko A. Rodriguez (http://markorodriguez.com)
*/
public class AddPropertyStep<S extends Element> extends SideEffectStep<S>
- implements AddPropertyStepContract<S>, Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent>, Configuring {
+ implements AddPropertyStepContract<S>, Writing<Event.ElementPropertyChangedEvent>, Deleting<Event.ElementPropertyChangedEvent> {
- private Parameters parameters = new Parameters();
+ private Parameters internalParameters = new Parameters();
+ private Parameters withConfiguration = new Parameters();
private final VertexProperty.Cardinality cardinality;
private CallbackRegistry<Event.ElementPropertyChangedEvent> callbackRegistry;
public AddPropertyStep(final Traversal.Admin traversal, final VertexProperty.Cardinality cardinality, final Object keyObject, final Object valueObject) {
super(traversal);
- this.parameters.set(this, T.key, keyObject, T.value, valueObject);
+ this.internalParameters.set(this, T.key, keyObject, T.value, valueObject);
this.cardinality = cardinality;
}
@Override
public Parameters getParameters() {
- return this.parameters;
+ return this.withConfiguration;
}
@Override
public Set<String> getScopeKeys() {
- return this.parameters.getReferencedLabels();
+ return this.internalParameters.getReferencedLabels();
}
@Override
public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
- return this.parameters.getTraversals();
+ return this.internalParameters.getTraversals();
}
@Override
public void configure(final Object... keyValues) {
- this.parameters.set(this, keyValues);
+ this.withConfiguration.set(this, keyValues);
+ }
+
+ private void configureInternalParams(final Object... keyValues) {
Review Comment:
Nit: this method is just one line and called once so not very useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380289292
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddVertexStepTest.java:
##########
Review Comment:
Can you please add unit test coverage for other affected steps ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer merged PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380258105
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStepPlaceholder.java:
##########
@@ -60,6 +61,8 @@ public class AddPropertyStepPlaceholder<S extends Element> extends AbstractStep<
*/
private Map<Object, List<Object>> properties = new HashMap<>();
+ private Parameters withConfiguration = new Parameters();
Review Comment:
Can this contain GValues or traversals?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2382930909
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java:
##########
@@ -54,6 +55,7 @@ public abstract class AbstractAddElementStepPlaceholder<S, E extends Element, X
protected Map<Object, List<Object>> properties = new HashMap<>();
protected GValue<Object> elementId;
protected Set<String> scopeKeys = new HashSet<>();
+ protected Parameters withConfiguration = new Parameters();
Review Comment:
Yes to traversals, this is required for with() modulation. There is currently no intention to allow GValue through with()-modulation, I will check to ensure this is explicitly blocked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380123864
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java:
##########
@@ -318,7 +325,17 @@ public AbstractAddElementStepPlaceholder<S, E, X> clone() {
clone.elementId = this.elementId;
clone.scopeKeys = new HashSet<>(this.scopeKeys);
+ clone.withConfiguration = this.withConfiguration.clone();
Review Comment:
`hashCode` should also be modified to account for the new `withConfiguration`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380128814
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java:
##########
@@ -54,6 +55,7 @@ public abstract class AbstractAddElementStepPlaceholder<S, E extends Element, X
protected Map<Object, List<Object>> properties = new HashMap<>();
protected GValue<Object> elementId;
protected Set<String> scopeKeys = new HashSet<>();
+ protected Parameters withConfiguration = new Parameters();
Review Comment:
Can these config `Parameters` contain GValues or traversals?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380015583
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -888,6 +874,41 @@ pinned variable is one which must remain bound to the current value. By paramete
variable, as it can be substituted in the optimized traversal without any loss of meaning, however the variable "count"
will be pinned as the optimized traversal is not valid for other values of "count".
+===== Decouple internal step state from `Configuring`/`Parameterizing` interfaces
+
+Previously `AddVertexStep`, `AddVertexStartStep`, `AddEdgeStep`, `AddEdgeStartStep`, and `AddPropertyStep` stored their
+internal state (id, label, properties, from/to vertices...) in a `Parameters` object which was exposed via the
+`Configuring` and `Parameterizing` interfaces. These interfaces are primarily intended to support with()-modulation, and
+thus these steps had weird unintended behaviors when combined with with()-modulation:
+
+[source,text]
+----
+gremlin> g.addV().with("name", "cole").valueMap()
+==>[name:[cole]]
Review Comment:
What is this example intended to inform? Is it just an example of usage of `with`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380116695
##########
docs/src/upgrade/release-3.8.x.asciidoc:
##########
@@ -888,6 +874,41 @@ pinned variable is one which must remain bound to the current value. By paramete
variable, as it can be substituted in the optimized traversal without any loss of meaning, however the variable "count"
will be pinned as the optimized traversal is not valid for other values of "count".
+===== Decouple internal step state from `Configuring`/`Parameterizing` interfaces
+
+Previously `AddVertexStep`, `AddVertexStartStep`, `AddEdgeStep`, `AddEdgeStartStep`, and `AddPropertyStep` stored their
+internal state (id, label, properties, from/to vertices...) in a `Parameters` object which was exposed via the
+`Configuring` and `Parameterizing` interfaces. These interfaces are primarily intended to support with()-modulation, and
+thus these steps had weird unintended behaviors when combined with with()-modulation:
+
+[source,text]
+----
+gremlin> g.addV().with("name", "cole").valueMap()
+==>[name:[cole]]
+----
+
+This internal step state has now been decoupled from the `Configuring` and `Parameterizing` interfaces, which are now
+exclusively used for the purposes of with()-modulation. All accesses of these steps internal data must now go through
Review Comment:
Can you be a bit more explicit about how users can identify which traversals need to be updated before upgrading?
Also it appears the breaking change affects not only users executing queries but also providers as well who are reading/modifying step data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [TINKERPOP-3193] Decouple non with()-related configs from Configuring/Parameterizing interfaces [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380120090
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java:
##########
@@ -54,6 +55,7 @@ public abstract class AbstractAddElementStepPlaceholder<S, E extends Element, X
protected Map<Object, List<Object>> properties = new HashMap<>();
protected GValue<Object> elementId;
protected Set<String> scopeKeys = new HashSet<>();
+ protected Parameters withConfiguration = new Parameters();
Review Comment:
Can you add guidance to the `Configuring.java` interface as to when the config data should be stored separately from other data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org