TINKERPOP-2235 Refactored Scoping interface
Pulled back all functionality that grabs scope values to Scoping. In this way we have just one place for this logic. Also decided to use exceptions to deal with keys not being found. Not thinking the performance costs of the exception will matter here much as we won't be generating exceptions repeatedly for purpose of flow control - we really just generate them when there is actual error in the traversal syntax and the traversal is headed for exception anyway. if we find that this is a mistaken understanding we can go the way of FastNotSuchElementException and deal with things that way. The alternative would have been to deal with some kind of new return object for the Scoping methods that could indicate whether the key was present or not, but that approach seemed awkward in the various ways I considered it.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
index dee401c..e2c0e17 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
@@ -24,7 +24,6 @@
import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
import java.util.Map;
-import java.util.Optional;
import java.util.Set;
/**
@@ -110,28 +109,54 @@
public enum Variable {START, END}
- public default <S> S getScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser) throws IllegalArgumentException {
- return Optional.<S>ofNullable(getNullableScopeValue(pop, key, traverser)).orElseThrow(
- () -> new IllegalArgumentException("Neither the sideEffects, map, nor path has a " + key + "-key: " + this));
- }
-
- public default <S> S getNullableScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser) {
- return getScopeValue(pop, key, traverser, null);
- }
-
- public default <S> S getScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser, final S valueIfNotFound) {
+ /**
+ * Finds the object with the specified key for the current traverser and throws an exception if the key cannot
+ * be found.
+ *
+ * @throws KeyNotFoundException if the key does not exist
+ */
+ public default <S> S getScopeValue(final Pop pop, final Object key, final Traverser.Admin<?> traverser) throws KeyNotFoundException {
final Object object = traverser.get();
- if (object instanceof Map && ((Map<String, S>) object).containsKey(key))
- return ((Map<String, S>) object).get(key);
- ///
- if (traverser.getSideEffects().exists(key))
- return traverser.getSideEffects().get(key);
- ///
- final Path path = traverser.path();
- if (path.hasLabel(key))
- return path.get(pop, key);
- ///
- return valueIfNotFound;
+ if (object instanceof Map && ((Map) object).containsKey(key))
+ return (S) ((Map) object).get(key);
+
+ if (key instanceof String) {
+ final String k = (String) key;
+ if (traverser.getSideEffects().exists(k))
+ return traverser.getSideEffects().get(k);
+
+ final Path path = traverser.path();
+ if (path.hasLabel(k))
+ return null == pop ? path.get(k) : path.get(pop, k);
+ }
+
+ throw new KeyNotFoundException(key, this);
+ }
+
+ /**
+ * Calls {@link #getScopeValue(Pop, Object, Traverser.Admin)} but throws an unchecked {@code IllegalStateException}
+ * if the key cannot be found.
+ */
+ public default <S> S getSafeScopeValue(final Pop pop, final Object key, final Traverser.Admin<?> traverser) {
+ try {
+ return getScopeValue(pop, key, traverser);
+ } catch (KeyNotFoundException nfe) {
+ throw new IllegalArgumentException(nfe.getMessage(), nfe);
+ }
+ }
+
+ /**
+ * Calls {@link #getScopeValue(Pop, Object, Traverser.Admin)} and returns {@code null} if the key is not found.
+ * Use this method with caution as {@code null} has one of two meanings as a return value. It could be that the
+ * key was found and its value was {@code null} or it might mean that the key was not found and {@code null} was
+ * simply returned.
+ */
+ public default <S> S getNullableScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser) {
+ try {
+ return getScopeValue(pop, key, traverser);
+ } catch (KeyNotFoundException nfe) {
+ return null;
+ }
}
/**
@@ -140,4 +165,24 @@
* @return the accessed labels of the scoping step
*/
public Set<String> getScopeKeys();
+
+ public static class KeyNotFoundException extends Exception {
+
+ private final Object key;
+ private final Scoping step;
+
+ public KeyNotFoundException(final Object key, final Scoping current) {
+ super("Neither the map, sideEffects, nor path has a " + key + "-key: " + current);
+ this.key = key;
+ this.step = current;
+ }
+
+ public Object getKey() {
+ return key;
+ }
+
+ public Scoping getStep() {
+ return step;
+ }
+ }
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java
index b4f70d9..911d2eb 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java
@@ -74,7 +74,7 @@
return this.duplicateSet.add(TraversalUtil.applyNullable(traverser, this.dedupTraversal));
} else {
final List<Object> objects = new ArrayList<>(this.dedupLabels.size());
- this.dedupLabels.forEach(label -> objects.add(TraversalUtil.applyNullable((S) this.getScopeValue(Pop.last, label, traverser), this.dedupTraversal)));
+ this.dedupLabels.forEach(label -> objects.add(TraversalUtil.applyNullable((S) this.getSafeScopeValue(Pop.last, label, traverser), this.dedupTraversal)));
return this.duplicateSet.add(objects);
}
}
@@ -189,7 +189,7 @@
if (null != this.dedupLabels) {
object = new ArrayList<>(this.dedupLabels.size());
for (final String label : this.dedupLabels) {
- ((List) object).add(TraversalUtil.applyNullable((S) this.getScopeValue(Pop.last, label, traverser), this.dedupTraversal));
+ ((List) object).add(TraversalUtil.applyNullable((S) this.getSafeScopeValue(Pop.last, label, traverser), this.dedupTraversal));
}
} else {
object = TraversalUtil.applyNullable(traverser, this.dedupTraversal);
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java
index 240a4cc..62a88f4 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WherePredicateStep.java
@@ -79,7 +79,7 @@
if (predicate instanceof ConnectiveP)
((ConnectiveP<Object>) predicate).getPredicates().forEach(p -> this.setPredicateValues(p, traverser, selectKeysIterator));
else
- predicate.setValue(TraversalUtil.applyNullable((S) this.getScopeValue(Pop.last, selectKeysIterator.next(), traverser), this.traversalRing.next()));
+ predicate.setValue(TraversalUtil.applyNullable((S) this.getSafeScopeValue(Pop.last, selectKeysIterator.next(), traverser), this.traversalRing.next()));
}
public Optional<P<?>> getPredicate() {
@@ -99,7 +99,7 @@
protected boolean filter(final Traverser.Admin<S> traverser) {
final Object value = null == this.startKey ?
TraversalUtil.applyNullable(traverser, this.traversalRing.next()) :
- TraversalUtil.applyNullable((S) this.getScopeValue(Pop.last, this.startKey, traverser), this.traversalRing.next());
+ TraversalUtil.applyNullable((S) this.getSafeScopeValue(Pop.last, this.startKey, traverser), this.traversalRing.next());
this.setPredicateValues(this.predicate, traverser, this.selectKeys.iterator());
this.traversalRing.reset();
return this.predicate.test(value);
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java
index 8c701a9..264575d 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/WhereTraversalStep.java
@@ -164,7 +164,7 @@
((WhereEndStep) this.getTraversal().getEndStep()).processStartTraverser(traverser);
else if (this.getTraversal().getEndStep() instanceof ProfileStep && this.getTraversal().getEndStep().getPreviousStep() instanceof WhereEndStep) // TOTAL SUCKY HACK!
((WhereEndStep) this.getTraversal().getEndStep().getPreviousStep()).processStartTraverser(traverser);
- return null == this.selectKey ? traverser.get() : this.getScopeValue(Pop.last, this.selectKey, traverser);
+ return null == this.selectKey ? traverser.get() : this.getSafeScopeValue(Pop.last, this.selectKey, traverser);
}
@Override
@@ -199,7 +199,7 @@
public void processStartTraverser(final Traverser.Admin traverser) {
if (null != this.matchKey)
- this.matchValue = this.getScopeValue(Pop.last, this.matchKey, traverser);
+ this.matchValue = this.getSafeScopeValue(Pop.last, this.matchKey, traverser);
}
@Override
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java
index 44e22fe..1f3ebdc 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectOneStep.java
@@ -18,7 +18,6 @@
*/
package org.apache.tinkerpop.gremlin.process.traversal.step.map;
-import org.apache.tinkerpop.gremlin.process.traversal.Path;
import org.apache.tinkerpop.gremlin.process.traversal.Pop;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
@@ -33,9 +32,7 @@
import java.util.Collections;
import java.util.HashSet;
-import java.util.Iterator;
import java.util.List;
-import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Set;
@@ -59,36 +56,16 @@
protected Traverser.Admin<E> processNextStart() throws NoSuchElementException {
final Traverser.Admin<S> traverser = this.starts.next();
- final Object object = traverser.get();
- if (object instanceof Map && ((Map<String, S>) object).containsKey(this.selectKey)) {
- final S o = ((Map<String, S>) object).get(this.selectKey);
+ try {
+ final S o = getScopeValue(pop, selectKey, traverser);
if (null == o) return traverser.split(null, this);
final Traverser.Admin<E> outTraverser = traverser.split(TraversalUtil.applyNullable(o, this.selectTraversal), this);
if (!(this.getTraversal().getParent() instanceof MatchStep))
PathProcessor.processTraverserPathLabels(outTraverser, this.keepLabels);
return outTraverser;
+ } catch (KeyNotFoundException nfe) {
+ return EmptyTraverser.instance();
}
-
- if (traverser.getSideEffects().exists(this.selectKey)) {
- final S o = (S) traverser.getSideEffects().get(this.selectKey);
- if (null == o) return traverser.split(null, this);
- final Traverser.Admin<E> outTraverser = traverser.split(TraversalUtil.applyNullable(o, this.selectTraversal), this);
- if (!(this.getTraversal().getParent() instanceof MatchStep))
- PathProcessor.processTraverserPathLabels(outTraverser, this.keepLabels);
- return outTraverser;
- }
-
- final Path path = traverser.path();
- if (path.hasLabel(this.selectKey)) {
- final S o = (S) path.get(pop, this.selectKey);
- if (null == o) return traverser.split(null, this);
- final Traverser.Admin<E> outTraverser = traverser.split(TraversalUtil.applyNullable(o, this.selectTraversal), this);
- if (!(this.getTraversal().getParent() instanceof MatchStep))
- PathProcessor.processTraverserPathLabels(outTraverser, this.keepLabels);
- return outTraverser;
- }
-
- return EmptyTraverser.instance();
}
@Override
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java
index 2fa3e1f..5034c84 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/SelectStep.java
@@ -66,16 +66,17 @@
protected Traverser.Admin<Map<String, E>> processNextStart() throws NoSuchElementException {
final Traverser.Admin<S> traverser = this.starts.next();
final Map<String, E> bindings = new LinkedHashMap<>(this.selectKeys.size(), 1.0f);
- for (final String selectKey : this.selectKeys) {
- final E end = this.getNullableScopeValue(this.pop, selectKey, traverser);
- if (null != end)
+ try {
+ for (final String selectKey : this.selectKeys) {
+ final E end = this.getScopeValue(this.pop, selectKey, traverser);
bindings.put(selectKey, TraversalUtil.applyNullable(end, this.traversalRing.next()));
- else {
- this.traversalRing.reset();
- return EmptyTraverser.instance();
}
+ } catch (KeyNotFoundException nfe) {
+ return EmptyTraverser.instance();
+ } finally {
+ this.traversalRing.reset();
}
- this.traversalRing.reset();
+
return PathProcessor.processTraverserPathLabels(traverser.split(bindings, this), this.keepLabels);
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TraversalSelectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TraversalSelectStep.java
index b1b1263..3813454 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TraversalSelectStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/TraversalSelectStep.java
@@ -18,12 +18,12 @@
*/
package org.apache.tinkerpop.gremlin.process.traversal.step.map;
-import org.apache.tinkerpop.gremlin.process.traversal.Path;
import org.apache.tinkerpop.gremlin.process.traversal.Pop;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating;
import org.apache.tinkerpop.gremlin.process.traversal.step.PathProcessor;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping;
import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.EmptyTraverser;
@@ -34,14 +34,12 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
-import java.util.Map;
import java.util.Set;
-
/**
* @author Daniel Kuppitz (http://gremlin.guru)
*/
-public final class TraversalSelectStep<S, E> extends MapStep<S, E> implements TraversalParent, PathProcessor, ByModulating {
+public final class TraversalSelectStep<S, E> extends MapStep<S, E> implements TraversalParent, PathProcessor, ByModulating, Scoping {
private final Pop pop;
private Traversal.Admin<S, E> keyTraversal;
@@ -57,43 +55,36 @@
@Override
protected Traverser.Admin<E> processNextStart() {
final Traverser.Admin<S> traverser = this.starts.next();
-
- boolean found = false;
- E end = null;
final Iterator<E> keyIterator = TraversalUtil.applyAll(traverser, this.keyTraversal);
if (keyIterator.hasNext()) {
final E key = keyIterator.next();
- final Object object = traverser.get();
- if (object instanceof Map && ((Map) object).containsKey(key)) {
- end = (E) ((Map) object).get(key);
- found = true;
- } else if (key instanceof String) {
- final String skey = (String) key;
- if (traverser.getSideEffects().exists(skey)) {
- end = traverser.getSideEffects().get((String) key);
- found = true;
- } else {
- final Path path = traverser.path();
- if (path.hasLabel(skey)) {
- end = null == pop ? path.get(skey) : path.get(pop, skey);
- found = true;
- }
+ try {
+ final E end = getScopeValue(pop, key, traverser);
+ final Traverser.Admin<E> outTraverser = traverser.split(null == end ? null : TraversalUtil.applyNullable(end, this.selectTraversal), this);
+ if (!(this.getTraversal().getParent() instanceof MatchStep)) {
+ PathProcessor.processTraverserPathLabels(outTraverser, this.keepLabels);
}
+ return outTraverser;
+ } catch (KeyNotFoundException nfe) {
+ return EmptyTraverser.instance();
}
- }
-
- if (found) {
- final Traverser.Admin<E> outTraverser = traverser.split(null == end ? null : TraversalUtil.applyNullable(end, this.selectTraversal), this);
- if (!(this.getTraversal().getParent() instanceof MatchStep)) {
- PathProcessor.processTraverserPathLabels(outTraverser, this.keepLabels);
- }
- return outTraverser;
} else {
return EmptyTraverser.instance();
}
}
@Override
+ public Set<String> getScopeKeys() {
+ // can't return scope keys here because they aren't known prior to traversal execution and this method is
+ // used at strategy application time. not getting any test failures as a result of returning empty. assuming
+ // that strategies don't use Scoping in a way that requires the keys to be known and if they aren't doesn't
+ // hose the whole traversal. in the worst case, strategies will hopefully just leave steps alone rather than
+ // make their own assumptions that the step is not selecting anything. if that is happening somehow we might
+ // need to modify Scoping to better suite this runtime evaluation of the key.
+ return Collections.emptySet();
+ }
+
+ @Override
public String toString() {
return StringFactory.stepString(this, this.pop, this.keyTraversal, this.selectTraversal);
}
diff --git a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs
index e81e00e..b747200 100644
--- a/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs
+++ b/gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs
@@ -38,7 +38,8 @@
{
{ "g.V().fold().count(Scope.local)", g => g.V().Fold().Count(Scope.Local)},
{ "g.inject(10,20,null,20,10,10).groupCount(\"x\").dedup().as(\"y\").project(\"a\",\"b\").by().by(__.select(\"x\").select(__.select(\"y\")))",
- g => g.Inject<object>(10,20,null,20,10,10).GroupCount("x").Dedup().As("y").Project<object>("a","b").By().By(__.Select<int>("x").Select<object>(__.Select<int>("y")))}
+ g => g.Inject<object>(10,20,null,20,10,10).GroupCount("x").Dedup().As("y").Project<object>("a","b").By().By(__.Select<int>("x").Select<object>(__.Select<int>("y")))},
+ { "g.inject(m).select(\"name\",\"age\")", g => g.Inject(new Dictionary<string,object>() {{"name", "marko"}, {"age", null}}).Select<object>("name", "age") }
};
private static readonly Regex RegexNumeric =
diff --git a/gremlin-test/features/sideEffect/Inject.feature b/gremlin-test/features/sideEffect/Inject.feature
index 7e4983d..20d6bd2 100644
--- a/gremlin-test/features/sideEffect/Inject.feature
+++ b/gremlin-test/features/sideEffect/Inject.feature
@@ -92,4 +92,16 @@
| result |
| m[{"a":"d[10].i", "b":"d[3].l"}] |
| m[{"a":"d[20].i", "b":"d[2].l"}] |
- | m[{"a":null, "b":"d[1].l"}] |
\ No newline at end of file
+ | m[{"a":null, "b":"d[1].l"}] |
+
+ Scenario: g_injectXname_marko_age_nullX_selectXname_ageX
+ Given the empty graph
+ And using the parameter m defined as "m[{\"name\":\"marko\", \"age\":null}]"
+ And the traversal of
+ """
+ g.inject(m).select("name","age")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | m[{"name":"marko", "age":null}] |
\ No newline at end of file
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/InjectTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/InjectTest.java
index 6ce50c9..fd752d2 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/InjectTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/InjectTest.java
@@ -53,6 +53,8 @@
public abstract Traversal<Integer, Map<String, Object>> get_g_injectX10_20_null_20_10_10X_groupCountXxX_dedup_asXyX_projectXa_bX_by_byXselectXxX_selectXselectXyXXX();
+ public abstract Traversal<Map<String,Object>, Map<String, Object>> get_g_injectXname_marko_age_nullX_selectXname_ageX();
+
@Test
@LoadGraphWith(MODERN)
public void g_VX1X_out_injectXv2X_name() {
@@ -107,6 +109,13 @@
}
@Test
+ public void g_injectXname_marko_age_nullX_selectXname_ageX() {
+ final Traversal<Map<String, Object>, Map<String, Object>> traversal = get_g_injectXname_marko_age_nullX_selectXname_ageX();
+ printTraversalForm(traversal);
+ checkResults(makeMapList(2, "name", "marko", "age", null), traversal);
+ }
+
+ @Test
@LoadGraphWith(MODERN)
public void g_VX1X_injectXg_VX4XX_out_name() {
final Traversal<Vertex, String> traversal = get_g_VX1X_injectXg_VX4XX_out_name(convertToVertexId("marko"), convertToVertexId("josh"));
@@ -144,5 +153,13 @@
by().
by(__.select("x").select(__.select("y")));
}
+
+ @Override
+ public Traversal<Map<String, Object>, Map<String, Object>> get_g_injectXname_marko_age_nullX_selectXname_ageX() {
+ final Map<String,Object> m = new HashMap<>();
+ m.put("name", "marko");
+ m.put("age", null);
+ return g.inject(m).select("name","age");
+ }
}
}
