Repository: tinkerpop Updated Branches: refs/heads/TINKERPOP-1490 bda502784 -> e07cfd06e (forced update)
fixed an NPE bug in profiling GroupSideEffectStep. It has to deal with how GroupSideEffectStep breaks apart the valueTraversal into a pre/post-traversal for pre-proecessing and reduction. While I was testing, I found another bug where if the reducing traversal has no data, then don't .next() it. Basically, it is possible for Profiling to get a traversal that has not been next'd because the traversal's pre/post was being worked with. Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/3d70c3d1 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/3d70c3d1 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/3d70c3d1 Branch: refs/heads/TINKERPOP-1490 Commit: 3d70c3d15d4591acab84420f86991cd98a187c38 Parents: b4d0ef9 Author: Marko A. Rodriguez <[email protected]> Authored: Thu Nov 3 10:14:06 2016 -0600 Committer: Marko A. Rodriguez <[email protected]> Committed: Thu Nov 3 10:14:06 2016 -0600 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../process/traversal/step/map/GroupStep.java | 33 +++++---- .../step/sideEffect/GroupSideEffectStep.java | 11 +-- .../traversal/util/DefaultTraversalMetrics.java | 76 ++++++++++---------- .../step/sideEffect/GroovyGroupTest.groovy | 5 ++ .../traversal/step/sideEffect/GroupTest.java | 23 ++++++ 6 files changed, 95 insertions(+), 54 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d87c0fd..766b135 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima TinkerPop 3.2.4 (Release Date: NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Fixed a `NullPointerException` bug with profiling `GroupSideEffectStep` in OLTP. * Improved ability to release resources in `GraphProvider` instances in the test suite. * Added a `force` option for killing sessions without waiting for transaction close or timeout of a currently running job or multiple jobs. * Deprecated `Session.kill()` and `Session.manualKill()`. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java index 84c5ca8..737c5f9 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroupStep.java @@ -22,7 +22,6 @@ package org.apache.tinkerpop.gremlin.process.traversal.step.map; import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.Traverser; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ElementValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.FunctionTraverser; @@ -61,8 +60,8 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> private char state = 'k'; private Traversal.Admin<S, K> keyTraversal = null; - private Traversal.Admin<S, ?> preTraversal; - private Traversal.Admin<S, V> valueTraversal; + private Traversal.Admin<S, ?> preTraversal = null; + private Traversal.Admin<S, V> valueTraversal = null; public GroupStep(final Traversal.Admin traversal) { super(traversal); @@ -109,10 +108,12 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> @Override public List<Traversal.Admin<?, ?>> getLocalChildren() { - final List<Traversal.Admin<?, ?>> children = new ArrayList<>(2); + final List<Traversal.Admin<?, ?>> children = new ArrayList<>(3); if (null != this.keyTraversal) - children.add((Traversal.Admin) this.keyTraversal); + children.add(this.keyTraversal); children.add(this.valueTraversal); + if (null != this.preTraversal) + children.add(this.preTraversal); return children; } @@ -127,7 +128,8 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> if (null != this.keyTraversal) clone.keyTraversal = this.keyTraversal.clone(); clone.valueTraversal = this.valueTraversal.clone(); - clone.preTraversal = this.integrateChild(GroupStep.generatePreTraversal(clone.valueTraversal)); + if (null != this.preTraversal) + clone.preTraversal = this.preTraversal.clone(); return clone; } @@ -332,26 +334,30 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> public static Traversal.Admin<?, ?> generatePreTraversal(final Traversal.Admin<?, ?> valueTraversal) { if (!TraversalHelper.hasStepOfAssignableClass(Barrier.class, valueTraversal)) - return valueTraversal; + return valueTraversal.clone(); final Traversal.Admin<?, ?> first = __.identity().asAdmin(); + boolean updated = false; for (final Step step : valueTraversal.getSteps()) { if (step instanceof Barrier) break; first.addStep(step.clone()); + updated = true; } - return first.getSteps().size() == 1 ? null : first; + return updated ? first : null; } public static <K, V> Map<K, V> doFinalReduction(final Map<K, Object> map, final Traversal.Admin<?, V> valueTraversal) { final Map<K, V> reducedMap = new HashMap<>(map.size()); final Barrier reducingBarrierStep = TraversalHelper.getFirstStepOfAssignableClass(Barrier.class, valueTraversal).orElse(null); IteratorUtils.removeOnNext(map.entrySet().iterator()).forEachRemaining(entry -> { - valueTraversal.reset(); if (null == reducingBarrierStep) { - reducedMap.put(entry.getKey(), entry.getValue() instanceof TraverserSet ? - ((TraverserSet<V>) entry.getValue()).iterator().next().get() : - (V) entry.getValue()); + if (entry.getValue() instanceof TraverserSet) { + if (!((TraverserSet) entry.getValue()).isEmpty()) + reducedMap.put(entry.getKey(), ((TraverserSet<V>) entry.getValue()).peek().get()); + } else + reducedMap.put(entry.getKey(), (V) entry.getValue()); } else { + valueTraversal.reset(); if (entry.getValue() instanceof Traverser.Admin) ((Step) reducingBarrierStep).addStart((Traverser.Admin) entry.getValue()); else if (entry.getValue() instanceof TraverserSet) @@ -361,7 +367,8 @@ public final class GroupStep<S, K, V> extends ReducingBarrierStep<S, Map<K, V>> reducingBarrierStep.addBarrier((((Pair) entry.getValue()).getValue1())); } else reducingBarrierStep.addBarrier(entry.getValue()); - reducedMap.put(entry.getKey(), valueTraversal.next()); + if (valueTraversal.hasNext()) + reducedMap.put(entry.getKey(), valueTraversal.next()); } }); assert map.isEmpty(); http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java index b5deb02..f2699e0 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupSideEffectStep.java @@ -45,7 +45,7 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem private char state = 'k'; private Traversal.Admin<S, K> keyTraversal = null; private Traversal.Admin<S, ?> preTraversal = null; - private Traversal.Admin<S, V> valueTraversal = this.integrateChild(__.fold().asAdmin()); + private Traversal.Admin<S, V> valueTraversal = null; /// private String sideEffectKey; @@ -99,10 +99,12 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem @Override public List<Traversal.Admin<?, ?>> getLocalChildren() { - final List<Traversal.Admin<?, ?>> children = new ArrayList<>(2); + final List<Traversal.Admin<?, ?>> children = new ArrayList<>(3); if (null != this.keyTraversal) - children.add((Traversal.Admin) this.keyTraversal); + children.add(this.keyTraversal); children.add(this.valueTraversal); + if (null != this.preTraversal) + children.add(this.preTraversal); return children; } @@ -117,7 +119,8 @@ public final class GroupSideEffectStep<S, K, V> extends SideEffectStep<S> implem if (null != this.keyTraversal) clone.keyTraversal = this.keyTraversal.clone(); clone.valueTraversal = this.valueTraversal.clone(); - clone.preTraversal = this.integrateChild(GroupStep.generatePreTraversal(clone.valueTraversal)); + if (null != this.preTraversal) + clone.preTraversal = this.preTraversal.clone(); return clone; } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversalMetrics.java ---------------------------------------------------------------------- diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversalMetrics.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversalMetrics.java index ce52ffc..ecf28e0 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversalMetrics.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/DefaultTraversalMetrics.java @@ -37,6 +37,7 @@ import java.util.concurrent.TimeUnit; /** * @author Bob Briody (http://bobbriody.com) + * @author Marko A. Rodriguez (http://markorodriguez.com) */ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializable { /** @@ -61,29 +62,29 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ */ public DefaultTraversalMetrics(final long totalStepDurationNs, final List<MutableMetrics> metricsMap) { this.totalStepDuration = totalStepDurationNs; - this.computedMetrics = new LinkedHashMap<>(metrics.size()); - metricsMap.forEach(m -> computedMetrics.put(m.getId(), m.getImmutableClone())); + this.computedMetrics = new LinkedHashMap<>(this.metrics.size()); + metricsMap.forEach(metric -> this.computedMetrics.put(metric.getId(), metric.getImmutableClone())); } @Override public long getDuration(final TimeUnit unit) { - return unit.convert(totalStepDuration, MutableMetrics.SOURCE_UNIT); + return unit.convert(this.totalStepDuration, MutableMetrics.SOURCE_UNIT); } @Override public Metrics getMetrics(final int index) { // adjust index to account for the injected profile steps - return (Metrics) computedMetrics.get(indexToLabelMap.get(index)); + return this.computedMetrics.get(this.indexToLabelMap.get(index)); } @Override public Metrics getMetrics(final String id) { - return computedMetrics.get(id); + return this.computedMetrics.get(id); } @Override public Collection<ImmutableMetrics> getMetrics() { - return computedMetrics.values(); + return this.computedMetrics.values(); } @Override @@ -96,7 +97,7 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ sb.append("\n============================================================================================================="); - appendMetrics(computedMetrics.values(), sb, 0); + appendMetrics(this.computedMetrics.values(), sb, 0); // Append total duration sb.append(String.format("%n%50s %21s %11s %15.3f %8s", @@ -150,25 +151,25 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ private void computeTotals() { // Create temp list of ordered metrics - List<MutableMetrics> tempMetrics = new ArrayList<>(metrics.size()); - for (String label : indexToLabelMap.values()) { + final List<MutableMetrics> tempMetrics = new ArrayList<>(this.metrics.size()); + for (final String label : this.indexToLabelMap.values()) { // The indexToLabelMap is sorted by index (key) - tempMetrics.add(metrics.get(label).clone()); + tempMetrics.add(this.metrics.get(label).clone()); } // Calculate total duration this.totalStepDuration = 0; - tempMetrics.forEach(m -> this.totalStepDuration += m.getDuration(MutableMetrics.SOURCE_UNIT)); + tempMetrics.forEach(metric -> this.totalStepDuration += metric.getDuration(MutableMetrics.SOURCE_UNIT)); // Assign %'s tempMetrics.forEach(m -> { - double dur = m.getDuration(TimeUnit.NANOSECONDS) * 100.d / this.totalStepDuration; + final double dur = m.getDuration(TimeUnit.NANOSECONDS) * 100.d / this.totalStepDuration; m.setAnnotation(PERCENT_DURATION_KEY, dur); }); // Store immutable instances of the calculated metrics - computedMetrics = new LinkedHashMap<>(metrics.size()); - tempMetrics.forEach(it -> computedMetrics.put(it.getId(), it.getImmutableClone())); + this.computedMetrics = new LinkedHashMap<>(this.metrics.size()); + tempMetrics.forEach(it -> this.computedMetrics.put(it.getId(), it.getImmutableClone())); } public static DefaultTraversalMetrics merge(final Iterator<DefaultTraversalMetrics> toMerge) { @@ -186,7 +187,7 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ newTraversalMetrics.metrics.put(metricsId, aggregateMetrics); // Set the index of the Metrics - for (Map.Entry<Integer, String> entry : inTraversalMetrics.indexToLabelMap.entrySet()) { + for (final Map.Entry<Integer, String> entry : inTraversalMetrics.indexToLabelMap.entrySet()) { if (metricsId.equals(entry.getValue())) { newTraversalMetrics.indexToLabelMap.put(entry.getKey(), metricsId); break; @@ -199,7 +200,7 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ return newTraversalMetrics; } - public void setMetrics(Traversal.Admin traversal, boolean onGraphComputer) { + public void setMetrics(final Traversal.Admin traversal, final boolean onGraphComputer) { addTopLevelMetrics(traversal, onGraphComputer); handleNestedTraversals(traversal, null, onGraphComputer); computeTotals(); @@ -222,36 +223,37 @@ public final class DefaultTraversalMetrics implements TraversalMetrics, Serializ } } - private void handleNestedTraversals(Traversal.Admin traversal, MutableMetrics parentMetrics, boolean onGraphComputer) { + private void handleNestedTraversals(final Traversal.Admin traversal, final MutableMetrics parentMetrics, final boolean onGraphComputer) { long prevDur = 0; - for (int ii = 0; ii < traversal.getSteps().size(); ii++) { - Step step = (Step) traversal.getSteps().get(ii); - if (!(step instanceof ProfileStep)) { + for (int i = 0; i < traversal.getSteps().size(); i++) { + final Step step = (Step) traversal.getSteps().get(i); + if (!(step instanceof ProfileStep)) continue; - } final MutableMetrics metrics = onGraphComputer ? traversal.getSideEffects().get(step.getId()) : ((ProfileStep) step).getMetrics(); - if (!onGraphComputer) { - // subtract upstream duration. - long durBeforeAdjustment = metrics.getDuration(TimeUnit.NANOSECONDS); - // adjust duration - metrics.setDuration(metrics.getDuration(TimeUnit.NANOSECONDS) - prevDur, TimeUnit.NANOSECONDS); - prevDur = durBeforeAdjustment; - } - - if (parentMetrics != null) { - parentMetrics.addNested(metrics); - } + if (null != metrics) { // this happens when a particular branch never received a .next() call (the metrics were never initialized) + if (!onGraphComputer) { + // subtract upstream duration. + long durBeforeAdjustment = metrics.getDuration(TimeUnit.NANOSECONDS); + // adjust duration + metrics.setDuration(metrics.getDuration(TimeUnit.NANOSECONDS) - prevDur, TimeUnit.NANOSECONDS); + prevDur = durBeforeAdjustment; + } - if (step.getPreviousStep() instanceof TraversalParent) { - for (Traversal.Admin<?, ?> t : ((TraversalParent) step.getPreviousStep()).getLocalChildren()) { - handleNestedTraversals(t, metrics, onGraphComputer); + if (parentMetrics != null) { + parentMetrics.addNested(metrics); } - for (Traversal.Admin<?, ?> t : ((TraversalParent) step.getPreviousStep()).getGlobalChildren()) { - handleNestedTraversals(t, metrics, onGraphComputer); + + if (step.getPreviousStep() instanceof TraversalParent) { + for (Traversal.Admin<?, ?> t : ((TraversalParent) step.getPreviousStep()).getLocalChildren()) { + handleNestedTraversals(t, metrics, onGraphComputer); + } + for (Traversal.Admin<?, ?> t : ((TraversalParent) step.getPreviousStep()).getGlobalChildren()) { + handleNestedTraversals(t, metrics, onGraphComputer); + } } } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroovyGroupTest.groovy ---------------------------------------------------------------------- diff --git a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroovyGroupTest.groovy b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroovyGroupTest.groovy index 13802b8..84da296 100644 --- a/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroovyGroupTest.groovy +++ b/gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroovyGroupTest.groovy @@ -118,5 +118,10 @@ public abstract class GroovyGroupTest { public Traversal<Vertex, Map<String, Map<String, Number>>> get_g_V_outXfollowedByX_group_byXsongTypeX_byXbothE_group_byXlabelX_byXweight_sumXX() { new ScriptTraversal<>(g, "gremlin-groovy", "g.V.out('followedBy').group.by('songType').by(bothE().group.by(label).by(values('weight').sum))") } + + @Override + public Traversal<Vertex, Map<String, String>> get_g_V_groupXmX_byXnameX_byXinXknowsX_nameX_capXmX() { + new ScriptTraversal<>(g, "gremlin-groovy", "g.V.group('m').by('name').by(__.in('knows').name).cap('m')") + } } } http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3d70c3d1/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java ---------------------------------------------------------------------- diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java index 356eb58..036c8c8 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/GroupTest.java @@ -88,6 +88,8 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { public abstract Traversal<Vertex, Map<String, Map<String, Number>>> get_g_V_outXfollowedByX_group_byXsongTypeX_byXbothE_group_byXlabelX_byXweight_sumXX(); + public abstract Traversal<Vertex, Map<String, String>> get_g_V_groupXmX_byXnameX_byXinXknowsX_nameX_capXmX(); + @Test @LoadGraphWith(MODERN) public void g_V_group_byXnameX() { @@ -364,6 +366,7 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { @LoadGraphWith(MODERN) public void g_V_group_byXbothE_countX_byXgroup_byXlabelXX() { final Traversal<Vertex, Map<Long, Map<String, List<Vertex>>>> traversal = get_g_V_group_byXbothE_countX_byXgroup_byXlabelXX(); + printTraversalForm(traversal); final Map<Long, Map<String, List<Vertex>>> map = traversal.next(); assertFalse(traversal.hasNext()); assertEquals(2, map.size()); @@ -399,6 +402,7 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { @LoadGraphWith(GRATEFUL) public void g_V_outXfollowedByX_group_byXsongTypeX_byXbothE_group_byXlabelX_byXweight_sumXX() { final Traversal<Vertex, Map<String, Map<String, Number>>> traversal = get_g_V_outXfollowedByX_group_byXsongTypeX_byXbothE_group_byXlabelX_byXweight_sumXX(); + printTraversalForm(traversal); final Map<String, Map<String, Number>> map = traversal.next(); assertFalse(traversal.hasNext()); assertEquals(3, map.size()); @@ -423,6 +427,20 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { assertEquals(0, subMap.get("sungBy").intValue()); } + @Test + @LoadGraphWith(MODERN) + public void g_V_groupXmX_byXnameX_byXinXknowsX_nameX_capXmX() { + final Traversal<Vertex, Map<String, String>> traversal = get_g_V_groupXmX_byXnameX_byXinXknowsX_nameX_capXmX(); + printTraversalForm(traversal); + final Map<String, String> map = traversal.next(); + assertFalse(traversal.hasNext()); + assertEquals(2, map.size()); + assertEquals("marko", map.get("vadas")); + assertEquals("marko", map.get("josh")); + + checkSideEffects(traversal.asAdmin().getSideEffects(), "m", HashMap.class); + } + public static class Traversals extends GroupTest { @Override @@ -514,5 +532,10 @@ public abstract class GroupTest extends AbstractGremlinProcessTest { public Traversal<Vertex, Map<String, Map<String, Number>>> get_g_V_outXfollowedByX_group_byXsongTypeX_byXbothE_group_byXlabelX_byXweight_sumXX() { return g.V().out("followedBy").<String, Map<String, Number>>group().by("songType").by(__.bothE().group().by(T.label).by(__.values("weight").sum())); } + + @Override + public Traversal<Vertex, Map<String, String>> get_g_V_groupXmX_byXnameX_byXinXknowsX_nameX_capXmX() { + return g.V().group("m").by("name").by(__.in("knows").values("name")).cap("m"); + } } }
