You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2019/03/11 15:32:04 UTC
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
spmallette commented on a change in pull request #1076: TINKERPOP-2112 Fold property() so that T values can work in any order
URL: https://github.com/apache/tinkerpop/pull/1076#discussion_r264289732
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
##########
@@ -2081,8 +2081,20 @@ else if (value instanceof Traversal)
// if it can be detected that this call to property() is related to an addV/E() then we can attempt to fold
// the properties into that step to gain an optimization for those graphs that support such capabilities.
- final Step endStep = this.asAdmin().getEndStep();
- if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep || endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
+ Step endStep = this.asAdmin().getEndStep();
+
+ // always try to fold the property() into the initial "AddElementStep" as the performance will be better
+ // and as it so happens with T the value must be set by way of that approach otherwise you get an error.
+ // it should be safe to execute this loop this way as we'll either hit an "AddElementStep" or an "EmptyStep".
+ // if empty, it will just use the regular AddPropertyStep being tacked on to the end of the traversal as usual
+ while (endStep instanceof AddPropertyStep) {
+ endStep = endStep.getPreviousStep();
+ }
+
+ // it's possible to fold the property() into the Mutating step if there are no metaproperties (i.e. keyValues)
+ // and if the cardinality is unspecified
+ if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep ||
+ endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
keyValues.length == 0 && null == cardinality) {
Review comment:
Here's what is weird when I tried this kind of thing when i was working on the PR. We basically want to fold any `single` cardinality key/value pair, so for that the logic would be:
```java
if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep ||
endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
keyValues.length == 0 &&
(key instanceof T || (key instanceof String &&
((cardinality == VertexProperty.Cardinality.single) ||
(null == cardinality && asAdmin().getGraph().orElse(EmptyGraph.instance()).
features().vertex().getCardinality((String) key) == VertexProperty.Cardinality.single))))) {
((Mutating) endStep).addPropertyMutations(key, value);
} else {
this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value));
((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues);
}
```
The problem, which will occur for graphs like TinkerGraph which don't have a schema, is that:
```text
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).profile()
==>Traversal Metrics
Step Count Traversers Time (ms) % Dur
=============================================================================================================
AddVertexStartStep({k=[1, 2]}) 1 1 0.138 100.00
>TOTAL - - 0.138 -
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).valueMap()
==>[k:[1,2]]
```
If the user specifies the `single` and the key/values gets folded, the specified cardinality is lost and the graph just does whatever it wants with with the arguments given to `Graph.addVertex()`. For TinkerGraph, its default cardinality is `list` so despite the user saying otherwise, it goes in as `list`. Because the old logic uses `AddPropertyStep` the cardinality is preserved as an argument and things work according to how the user specified them.
I also tried this logic (which only takes into account the cardinality check you suggested):
```java
if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep ||
endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
keyValues.length == 0 &&
(key instanceof T || (key instanceof String &&
(null == cardinality && asAdmin().getGraph().orElse(EmptyGraph.instance()).
features().vertex().getCardinality((String) key) == VertexProperty.Cardinality.single)))) {
((Mutating) endStep).addPropertyMutations(key, value);
} else {
this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value));
((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues);
}
```
and that seemed to work a little better as `single` gets preserved properly:
```text
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).profile()
==>Traversal Metrics
Step Count Traversers Time (ms) % Dur
=============================================================================================================
AddVertexStartStep({}) 1 1 0.081 31.67
AddPropertyStep({key=[k], value=[1]}) 1 1 0.089 34.66
AddPropertyStep({key=[k], value=[2]}) 1 1 0.086 33.67
>TOTAL - - 0.257 -
gremlin>
gremlin> g = TinkerGraph.open().traversal()
==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).valueMap()
==>[k:[2]]
```
wdyt?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services