TINKERPOP-2971 Fixed bug in value traversal to group()

Ensured that reducing/supplying barriers that follow collecting barriers get some respect.

Author: spmallette

CHANGELOG.asciidoc

@@ -44,6 +44,9 @@ This release also includes changes from <<release-3-7-XXX, 3.7.XXX>>.
 * Added static `instance()` method to `ElementIdStrategy` to an instance with the default configuration.
 * Updated `ElementIdStrategy.getConfiguration()` to help with serialization.
 * Changed type for `ReservedKeysVerificationStrategy.keys` in .NET to take a `Set<string>` rather than `List<string>`.
+* Fixed bug in `group()` value traversal of the second `by()` where a `CollectingBarrierStep` could produce an unexpected filtering effect when `ReducingBarrierStep` or `SupplyingBarrierStep` instances were not taken into account.
+* Changed `DetachedFactory` to special case the handling of `ComputerAdjacentVertex` which doesn't carry properties but still needs to be detachable for OLAP cases.
+* Deprecated `ProfilingAware.prepareForProfiling` method preferring to simply `resetBarrierFromValueTraversal` from the `Grouping` interface after strategy application.
 
 == TinkerPop 3.7.0 (Gremfir Master of the Pan Flute)
 

docs/src/upgrade/release-3.8.x.asciidoc

@@ -174,10 +174,42 @@ Integer overflows caused by addition and multiplication operations will throw an
 skipped with incorrect result.
 
 ==== Gremlin Lang Float Literals Default to Double
+
 The `GremlinLangScriptEngine` has been modified to treat float literals without explicit type suffixes (like 'm', 'f',
 or 'd') as Double by default. Users who need BigDecimal precision can still use the 'm' suffix (e.g., 1.0m). 
 `GremlinGroovyScriptEngine` will still default to BigDecimal for float literals.
 
+==== group() Value Traversal Semantics
+
+The `group()` step takes two `by()` modulators. The first defines the key for the grouping and the second acts upon the
+values that were grouped to each key. The latter is referred to as the "value traversal". In earlier versions of
+TinkerPop, using `order()` in the value traversal could produce an unexpected result if combined with a step like
+`fold()`.
+
+[source,text]
+----
+gremlin> g.V().has("person","name",P.within("vadas","peter")).group().by().by(__.out().fold())
+==>[v[2]:[],v[6]:[v[3]]]
+gremlin> g.V().has("person","name",P.within("vadas","peter")).group().by().by(__.out().order().fold())
+==>[v[6]:[v[3]]]
+----
+
+The example above shows that `v[2]` gets filtered away when `order()` is included. Obviously, this was not expected
+behavior. The problem can be more generally explained as an issue where a `Barrier` like `order()` can return an empty
+result. If this step is followed by another `Barrier` that always produces an output like `sum()`, `count()` or `fold()`
+then the empty result would not feed through to that following step. This issue has now been fixed and the two
+traversals from the previous example now return the same results.
+
+[source,text]
+----
+gremlin> g.V().has("person","name",P.within("vadas","peter")).group().by().by(__.out().fold())
+==>[v[2]:[],v[6]:[v[3]]]
+gremlin> g.V().has("person","name",P.within("vadas","peter")).group().by().by(__.out().order().fold())
+==>[v[2]:[],v[6]:[v[3]]]
+----
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2971[TINKERPOP-2971]
+
 === Upgrading for Providers
 
 ==== Graph System Providers

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/util/ComputerGraph.java

@@ -49,7 +49,7 @@
  */
 public final class ComputerGraph implements Graph {
 
-    private enum State {VERTEX_PROGRAM, MAP_REDUCE}
+    public enum State {VERTEX_PROGRAM, MAP_REDUCE}
 
     private ComputerVertex starVertex;
     private final Set<String> computeKeys;

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Barrier.java

@@ -73,4 +73,11 @@ public interface Barrier<B> extends MemoryComputing<B> {
     public default void done() {
 
     }
+
+    /**
+     * If a barrier is unproductive then provide an empty object suitable to the implementation which can be used
+     * to represent that state. This is important for cases like {@code by(out().order().fold())} where the
+     * {@code order()} might filter but the {@code fold()} means the traversal is productive.
+     */
+    public B getEmptyBarrier();
 }

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/FilteringBarrier.java

@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.step;
+
+import org.apache.tinkerpop.gremlin.process.traversal.step.filter.RangeGlobalStep;
+
+/**
+ * Marker interface for a {@link Barrier} that can behave as a filter, like {@link RangeGlobalStep}
+ */
+public interface FilteringBarrier<S> extends Barrier<S> {
+}

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Grouping.java

@@ -56,6 +56,13 @@ public default boolean hasBarrierInValueTraversal() {
         return null != determineBarrierStep(getValueTraversal());
     }
 
+    /**
+     * The first non-local {@link Barrier} step ascertained by calls to {@link #determineBarrierStep(Traversal.Admin)}
+     * is cached for a {@code Grouping}. After strategy application it is important that the cache be reset to ensure
+     * that any modifications to the child traversal are accounted for.
+     */
+    public void resetBarrierFromValueTraversal();
+
     /**
      * Determines the first non-local {@link Barrier} step in the provided traversal. This method is used by {@link GroupStep}
      * and {@link GroupSideEffectStep} to ultimately determine the reducing bi-operator.

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/LocalBarrier.java

@@ -33,4 +33,8 @@ public default MemoryComputeKey getMemoryComputeKey() {
         return MemoryComputeKey.of(((Step) this).getId(), Operator.and, false, true);
     }
 
+    @Override
+    default TraverserSet<S> getEmptyBarrier() {
+        return new TraverserSet<>();
+    }
 }

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/ProfilingAware.java

@@ -40,8 +40,13 @@ public interface ProfilingAware {
 
     /**
      * Prepares the step for any internal changes that might help ensure that profiling will work as expected.
+     * @deprecated As of release 3.8.0, not directly replaced because a simple call to {@link Step#reset()} at traversal
+     * lock can more generally do what is needed by this much more specific API.
      */
-    public void prepareForProfiling();
+    @Deprecated
+    public default void prepareForProfiling() {
+
+    }
 
     /**
      * A helper class which holds a {@link Barrier} and it's related {@link ProfileStep} so that the latter can have
@@ -85,6 +90,11 @@ public void addBarrier(final Object barrier) {
             this.barrier.addBarrier(barrier);
         }
 
+        @Override
+        public Object getEmptyBarrier() {
+            return new Object();
+        }
+
         @Override
         public MemoryComputeKey getMemoryComputeKey() {
             return this.barrier.getMemoryComputeKey();

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/DedupGlobalStep.java

@@ -23,14 +23,13 @@
 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.Barrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating;
+import org.apache.tinkerpop.gremlin.process.traversal.step.FilteringBarrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GraphComputing;
 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.util.FastNoSuchElementException;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalProduct;
 import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
 import org.apache.tinkerpop.gremlin.structure.Property;
@@ -53,7 +52,8 @@
 /**
  * @author Marko A. Rodriguez (http://markorodriguez.com)
  */
-public final class DedupGlobalStep<S> extends FilterStep<S> implements TraversalParent, Scoping, GraphComputing, Barrier<Map<Object, Traverser.Admin<S>>>, ByModulating, PathProcessor {
+public final class DedupGlobalStep<S> extends FilterStep<S> implements TraversalParent, Scoping, GraphComputing,
+        FilteringBarrier<Map<Object, Traverser.Admin<S>>>, ByModulating, PathProcessor {
 
     private Traversal.Admin<S, Object> dedupTraversal = null;
     private Set<Object> duplicateSet = new HashSet<>();
@@ -191,6 +191,11 @@ public void processAllStarts() {
 
     }
 
+    @Override
+    public Map<Object, Traverser.Admin<S>> getEmptyBarrier() {
+        return new HashMap<>();
+    }
+
     @Override
     public boolean hasNextBarrier() {
         return null != this.barrier || this.starts.hasNext();

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/filter/RangeGlobalStep.java

@@ -21,13 +21,12 @@
 import org.apache.tinkerpop.gremlin.process.computer.MemoryComputeKey;
 import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
 import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
-import org.apache.tinkerpop.gremlin.process.traversal.step.Barrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.Bypassing;
+import org.apache.tinkerpop.gremlin.process.traversal.step.FilteringBarrier;
 import org.apache.tinkerpop.gremlin.process.traversal.step.Ranging;
 import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
 import org.apache.tinkerpop.gremlin.process.traversal.traverser.util.TraverserSet;
 import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException;
-import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator;
 import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
 import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
 
@@ -42,7 +41,7 @@
  * @author Bob Briody (http://bobbriody.com)
  * @author Marko A. Rodriguez (http://markorodriguez.com)
  */
-public final class RangeGlobalStep<S> extends FilterStep<S> implements Ranging, Bypassing, Barrier<TraverserSet<S>> {
+public final class RangeGlobalStep<S> extends FilterStep<S> implements Ranging, Bypassing, FilteringBarrier<TraverserSet<S>> {
 
     private long low;
     private final long high;
@@ -155,6 +154,11 @@ public void processAllStarts() {
 
     }
 
+    @Override
+    public TraverserSet<S> getEmptyBarrier() {
+        return new TraverserSet<>();
+    }
+
     @Override
     public boolean hasNextBarrier() {
         return this.starts.hasNext();