Preserve BulkSet in P

Doesn't address all the problems here with performance but for now just trying to preserve BulkSet for within/without which can happen with: aggregate('x')...where(without('x')) CTR

Author: spmallette

CHANGELOG.asciidoc

@@ -25,6 +25,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
 
 This release also includes changes from <<release-3-7-6, 3.7.6>>.
 
+* Preserved `BulkSet` when given to 'P'.
 * Fixed bug in Gremlin orderability semantics for `OffsetDateTime`.
 * Fixed bug in Javascript translation for UUID.
 * Fixed bug in pre-repeat() `emit()/until()` where `emit()` and `until()` traversers weren't added to the results.

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

@@ -20,6 +20,7 @@
 
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.BulkSet;
 import org.apache.tinkerpop.gremlin.process.traversal.util.AndP;
 import org.apache.tinkerpop.gremlin.process.traversal.util.OrP;
 
@@ -71,7 +72,12 @@ public P(final PBiPredicate<V, V> biPredicate, final GValue<V> value) {
     protected P(final PBiPredicate<V, V> biPredicate, final Collection<V> literals, final Map<String, V> variables, final boolean isCollection) {
         this.biPredicate = biPredicate;
         this.variables.putAll(variables);
-        this.literals = new ArrayList<>(literals);
+        if (literals instanceof BulkSet) {
+            this.literals = new BulkSet<>();
+            this.literals.addAll(literals);
+        } else {
+            this.literals = new ArrayList<>(literals);
+        }
         this.isCollection = isCollection;
     }
 
@@ -89,7 +95,16 @@ public PBiPredicate<V, V> getBiPredicate() {
      */
     public V getValue() {
         if (isCollection) {
-            Collection<V> values = this.literals.stream().collect(Collectors.toList());
+            if (this.variables.isEmpty()) return (V) this.literals;
+
+            final Collection<V> values;
+            if (this.literals instanceof BulkSet) {
+                values = new BulkSet<>();
+                values.addAll(this.literals);
+            } else {
+                values = new ArrayList<>(this.literals);
+            }
+
             values.addAll(this.variables.values());
             return (V) values;
         } else if (!this.literals.isEmpty()) {
@@ -113,7 +128,7 @@ public void setValue(final V value) {
         } else if (value instanceof Collection) {
             isCollection = true;
             if (((Collection<?>) value).stream().anyMatch(v -> v instanceof GValue)) {
-                this.literals = new ArrayList<>();
+                this.literals = (value instanceof BulkSet) ? new BulkSet<>() : new ArrayList<>();
                 for (Object v : ((Collection<?>) value)) {
                     // Separate variables and literals
                     if (v instanceof GValue) {

gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/PBulkSetTest.java

@@ -0,0 +1,97 @@
+package org.apache.tinkerpop.gremlin.process.traversal;
+
+import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.BulkSet;
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Test class for {@link P} with {@link BulkSet} literals ensure that if a {@link BulkSet} is used as a literal,
+ * it is preserved in the {@link P} object's value, and that the {@link P} object can handle {@link BulkSet} literals
+ * with {@link GValue} elements correctly. You get a {@link BulkSet} in {@link P} when you have some Gremlin like:
+ * {@code aggreate('x')...where(within('x'))}.
+ */
+public class PBulkSetTest {
+
+    @Test
+    public void shouldPreserveBulkSetInGetValue() {
+        final BulkSet<String> bulkSet = new BulkSet<>();
+        bulkSet.add("a", 2);
+        bulkSet.add("b", 1);
+
+        final P p = P.within(bulkSet);
+        final Object value = p.getValue();
+
+        assertTrue("Value should be a BulkSet but was " + value.getClass().getName(), value instanceof BulkSet);
+        assertEquals(bulkSet, value);
+        assertEquals(3, ((BulkSet) value).size());
+        assertEquals(2L, ((BulkSet) value).get("a"));
+    }
+
+    @Test
+    public void shouldPreserveBulkSetInSetValueWithGValues() {
+        final BulkSet<Object> bulkSet = new BulkSet<>();
+        bulkSet.add("a", 2);
+        bulkSet.add(GValue.of("x", "b"), 1);
+
+        final P p = P.within(Collections.emptyList());
+        p.setValue(bulkSet);
+
+        final Object value = p.getValue();
+        // Currently this will fail and return an ArrayList (or a List from the stream)
+        assertTrue("Value should be a BulkSet but was " + (value == null ? "null" : value.getClass().getName()), value instanceof BulkSet);
+        assertEquals(3, ((BulkSet) value).size());
+        assertEquals(2L, ((BulkSet) value).get("a"));
+    }
+
+    @Test
+    public void shouldCloneBulkSetInGetValueWithVariables() {
+        final BulkSet<Object> bulkSet = new BulkSet<>();
+        bulkSet.add("a", 2);
+
+        // Subclass to access protected constructor and merge literals and variables
+        final P p = new PSubWithVars(Compare.eq, bulkSet, Collections.singletonMap("var1", "b"));
+
+        final Object value = p.getValue();
+        assertTrue("Value should be a BulkSet but was " + (value == null ? "null" : value.getClass().getName()), value instanceof BulkSet);
+        assertEquals(3, ((BulkSet) value).size());
+        assertEquals(2L, ((BulkSet) value).get("a"));
+        assertEquals(1L, ((BulkSet) value).get("b"));
+
+        // Ensure original bulkSet was not modified (p.getValue() should have cloned)
+        assertEquals(2, bulkSet.size());
+        assertEquals(2L, bulkSet.get("a"));
+        assertEquals(0L, bulkSet.get("b"));
+    }
+    
+    @Test
+    public void shouldPreserveBulkSetInProtectedConstructor() throws Exception {
+        final BulkSet<String> bulkSet = new BulkSet<>();
+        bulkSet.add("a", 2);
+        
+        final P p = new PSub(Compare.eq, bulkSet);
+        
+        final Object value = p.getValue();
+        assertTrue("Value should be a BulkSet but was " + (value == null ? "null" : value.getClass().getName()), value instanceof BulkSet);
+        assertEquals(2, ((BulkSet) value).size());
+        assertEquals(2L, ((BulkSet) value).get("a"));
+    }
+
+    private static class PSub extends P {
+        public PSub(final PBiPredicate biPredicate, final Collection literals) {
+            super(biPredicate, literals, Collections.emptyMap(), true);
+        }
+    }
+
+    private static class PSubWithVars extends P {
+        public PSubWithVars(final PBiPredicate biPredicate, final Collection literals, final Map variables) {
+            super(biPredicate, literals, variables, true);
+        }
+    }
+}