Delete refactoring (#153)

- `DeleteWalker` has two stages:
  1. Aggregate items for later deletion
  2. Deduplicate and perform deletion in side effect
- If RETURN depends on DELETE, 2 is executed as last action
  - In other cases, 2 is executed right after 1. These are separate actions because deduplication is important
- Custom predicate for filtering Node types
- For safety, delete for unknown types will fail without custom predicate, to avoid deletion of nodes with existing relationships
- Deduplicate before deletion to avoid errors on some databases
- TCK +1

Signed-off-by: Dwitry dwitry@users.noreply.github.com

testware/integration-tests/src/test/java/org/opencypher/gremlin/queries/DeleteTest.java

@@ -17,11 +17,15 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.tuple;
 
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.opencypher.gremlin.groups.SkipWithBytecode;
 import org.opencypher.gremlin.rules.GremlinServerExternalResource;
 
 public class DeleteTest {
@@ -33,6 +37,10 @@ private List<Map<String, Object>> submitAndGet(String cypher) {
         return gremlinServer.cypherGremlinClient().submit(cypher).all();
     }
 
+    private List<Map<String, Object>> submitAndGet(String cypher, Object... parameters) {
+        return gremlinServer.cypherGremlinClient().submit(cypher, parameterMap(parameters)).all();
+    }
+
     @Test
     public void delete() throws Exception {
         List<Map<String, Object>> beforeDelete = submitAndGet(
@@ -139,6 +147,81 @@ public void detachDeleteFromAList() throws Exception {
 
     @Test
     public void deleteWithReturn() throws Exception {
+        List<Map<String, Object>> onDelete = submitAndGet(
+            "MATCH (a:software) DETACH DELETE a RETURN a"
+        );
+
+        assertThat(onDelete)
+            .extracting("a")
+            .extracting("name")
+            .containsExactlyInAnyOrder("lop", "ripple");
+    }
+
+    @Test
+    public void deleteWithReturnOther() throws Exception {
+        List<Map<String, Object>> onDelete = submitAndGet(
+            "MATCH (a:software)<-[:created]-(p:person) DETACH DELETE a RETURN p"
+        );
+
+        assertThat(onDelete)
+            .extracting("p")
+            .extracting("name")
+            .containsExactlyInAnyOrder("marko", "josh", "josh", "peter");
+    }
+
+    @Test
+    public void deleteWithAggregationOnField() throws Exception {
+        List<Map<String, Object>> onDelete = submitAndGet(
+            "MATCH (a:software)<-[:created]-(p:person) DETACH DELETE a RETURN p.name, count(*)"
+        );
+
+        assertThat(onDelete)
+            .extracting("p.name", "count(*)")
+            .containsExactlyInAnyOrder(
+                tuple("marko", 1L),
+                tuple("josh", 2L),
+                tuple("peter", 1L));
+    }
+
+    @Test
+    public void deleteNothing() throws Exception {
+        List<Map<String, Object>> beforeDelete = submitAndGet(
+            "MATCH (n) RETURN count(*)"
+        );
+        List<Map<String, Object>> onDelete = submitAndGet(
+            "MATCH (a:notExisting) DELETE a RETURN a"
+        );
+        List<Map<String, Object>> afterDelete = submitAndGet(
+            "MATCH (n) RETURN count(*)"
+        );
+
+        assertThat(beforeDelete)
+            .extracting("count(*)")
+            .containsExactly(6L);
+        assertThat(onDelete)
+            .hasSize(0);
+        assertThat(afterDelete)
+            .extracting("count(*)")
+            .containsExactly(6L);
+    }
+
+    /**
+     * Custom predicate deserialization is not implemented
+     */
+    @Test
+    @Category(SkipWithBytecode.class)
+    public void deleteWithTypeLost() throws Exception {
+        assertThatThrownBy(() -> submitAndGet(
+            "MATCH (n) WITH collect(n) as typelost\n" +
+                "DELETE typelost[$i]",
+            "i",
+            0
+
+        )).hasMessageContaining("Cannot delete node, because it still has relationships.");
+    }
+
+    @Test
+    public void deleteWithReturnAggregation() throws Exception {
         submitAndGet("MATCH (n) DETACH DELETE n");
         submitAndGet("CREATE ()-[:R]->()");
         List<Map<String, Object>> beforeDelete = submitAndGet(
@@ -220,4 +303,12 @@ public void deleteConnectedNodeAndRelationship() {
             .extracting("count(*)")
             .containsExactly(5L);
     }
+
+    private HashMap<String, Object> parameterMap(Object[] parameters) {
+        HashMap<String, Object> result = new HashMap<>();
+        for (int i = 0; i < parameters.length; i+=2) {
+            result.put(String.valueOf(parameters[i]), parameters[i+1]);
+        }
+        return result;
+    }
 }

tinkerpop/cypher-gremlin-extensions/src/main/java/org/opencypher/gremlin/translation/Tokens.java

@@ -37,4 +37,8 @@ private Tokens() {
     public static final String GENERATED = "  GENERATED";
     public static final String UNNAMED = "  UNNAMED";
     public static final String FRESHID = "  FRESHID";
+
+    public static final String DELETE = "  cypher.delete";
+    public static final String DETACH_DELETE = "  cypher.delete.detach";
+    public static final String DELETE_ONCE = "  cypher.delete.once";
 }

tinkerpop/cypher-gremlin-extensions/src/main/java/org/opencypher/gremlin/traversal/CustomPredicate.java

@@ -17,6 +17,8 @@
 
 import java.util.function.BiPredicate;
 import org.apache.tinkerpop.gremlin.process.traversal.P;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+
 
 public enum CustomPredicate implements BiPredicate<Object, Object> {
     cypherStartsWith {
@@ -38,6 +40,13 @@ public boolean test(Object a, Object b) {
         public boolean test(Object a, Object b) {
             return a != null && b != null && a.toString().contains(b.toString());
         }
+    },
+
+    cypherIsNode {
+        @Override
+        public boolean test(Object a, Object b) {
+            return a instanceof Vertex;
+        }
     };
 
     public static P<Object> cypherStartsWith(final Object prefix) {
@@ -51,4 +60,8 @@ public static P<Object> cypherEndsWith(final Object suffix) {
     public static P<Object> cypherContains(final Object sequence) {
         return new P<>(CustomPredicate.cypherContains, sequence);
     }
+
+    public static P<Object> cypherIsNode() {
+        return new P<>(CustomPredicate.cypherIsNode, null);
+    }
 }

translation/src/main/java/org/opencypher/gremlin/translation/GremlinPredicates.java

@@ -51,4 +51,6 @@ public interface GremlinPredicates<P> {
     P endsWith(Object value);
 
     P contains(Object value);
+
+    P isNode();
 }

translation/src/main/java/org/opencypher/gremlin/translation/bytecode/BytecodeGremlinPredicates.java

@@ -83,6 +83,11 @@ public P contains(Object value) {
         return CustomPredicate.cypherContains(inlineParameter(value));
     }
 
+    @Override
+    public P isNode() {
+        return CustomPredicate.cypherIsNode();
+    }
+
     private static Object[] inlineParameters(Object... values) {
         return Stream.of(values)
             .map(BytecodeGremlinPredicates::inlineParameter)

translation/src/main/java/org/opencypher/gremlin/translation/groovy/GroovyGremlinPredicates.java

@@ -82,4 +82,9 @@ public GroovyPredicate endsWith(Object value) {
     public GroovyPredicate contains(Object value) {
         return new GroovyPredicate("cypherContains", value);
     }
+
+    @Override
+    public GroovyPredicate isNode() {
+        return new GroovyPredicate("cypherIsNode");
+    }
 }

translation/src/main/java/org/opencypher/gremlin/translation/traversal/TraversalGremlinPredicates.java

@@ -80,4 +80,9 @@ public P endsWith(Object value) {
     public P contains(Object value) {
         return CustomPredicate.cypherContains(value);
     }
+
+    @Override
+    public P isNode() {
+        return CustomPredicate.cypherIsNode();
+    }
 }

translation/src/main/scala/org/opencypher/gremlin/translation/ir/TranslationWriter.scala

@@ -265,6 +265,7 @@ sealed class TranslationWriter[T, P] private (translator: Translator[T, P], para
       case StartsWith(value)      => p.startsWith(writeValue(value))
       case EndsWith(value)        => p.endsWith(writeValue(value))
       case Contains(value)        => p.contains(writeValue(value))
+      case IsNode()               => p.isNode
     }
   }
 

translation/src/main/scala/org/opencypher/gremlin/translation/ir/TraversalHelper.scala

@@ -77,6 +77,22 @@ object TraversalHelper {
     findAcc(Nil, steps)
   }
 
+  /**
+    * Count occurrences in top-level and all nested traversals starting from the bottom and going up.
+    *
+    * @param extractor matching and mapping function
+    * @param steps top-level traversal
+    * @return map with number of occurrences of extractor result
+    */
+  def countInTraversals[R](extractor: PartialFunction[Seq[GremlinStep], R])(steps: Seq[GremlinStep]): Map[R, Int] = {
+    foldTraversals(Map.empty[R, Int])({ (acc, sub) =>
+      val labels = extract(extractor)(sub)
+      labels.foldLeft(acc) { (acc, label) =>
+        acc + ((label, acc.getOrElse(label, 0) + 1))
+      }
+    })(steps)
+  }
+
   /**
     * Finds matching parts of an IR sequence and replaces them.
     *

translation/src/main/scala/org/opencypher/gremlin/translation/ir/builder/IRGremlinPredicates.scala

@@ -42,4 +42,6 @@ class IRGremlinPredicates extends GremlinPredicates[GremlinPredicate] {
   override def endsWith(value: scala.Any): GremlinPredicate = EndsWith(value)
 
   override def contains(value: scala.Any): GremlinPredicate = Contains(value)
+
+  override def isNode: GremlinPredicate = IsNode()
 }

translation/src/main/scala/org/opencypher/gremlin/translation/ir/model/GremlinPredicate.scala

@@ -29,3 +29,4 @@ case class Without(values: Any*) extends GremlinPredicate
 case class StartsWith(value: Any) extends GremlinPredicate
 case class EndsWith(value: Any) extends GremlinPredicate
 case class Contains(value: Any) extends GremlinPredicate
+case class IsNode() extends GremlinPredicate

translation/src/main/scala/org/opencypher/gremlin/translation/ir/rewrite/NeptuneFlavor.scala

@@ -29,6 +29,7 @@ object NeptuneFlavor extends GremlinRewriter {
       injectWorkaround(_),
       limit0Workaround(_),
       multipleLabelsWorkaround(_),
+      aggregateWithSameNameWorkaround(_),
       traversalRewriters(_)
     ).foldLeft(steps) { (steps, rewriter) =>
       rewriter(steps)
@@ -61,6 +62,18 @@ object NeptuneFlavor extends GremlinRewriter {
     })(steps)
   }
 
+  private def aggregateWithSameNameWorkaround(steps: Seq[GremlinStep]): Seq[GremlinStep] = {
+    val aggregations = countInTraversals({
+      case Aggregate(sideEffectKey) :: _ => sideEffectKey
+    })(steps)
+
+    mapTraversals(replace({
+      case Aggregate(sideEffectKey) :: rest if aggregations(sideEffectKey) > 1 => {
+        SideEffect(Aggregate(sideEffectKey) :: Nil) :: rest
+      }
+    }))(steps)
+  }
+
   private def expandSub(key: String, bySteps: Seq[GremlinStep]): Seq[GremlinStep] = {
     replace({
       case By(expr, None) :: rest         => PropertyT(key, expr) +: expandSub(key, rest)

translation/src/main/scala/org/opencypher/gremlin/translation/ir/rewrite/RemoveMultipleAliases.scala

@@ -28,13 +28,8 @@ import scala.collection.JavaConverters._
 object RemoveMultipleAliases extends GremlinRewriter {
 
   override def apply(steps: Seq[GremlinStep]): Seq[GremlinStep] = {
-    val aliasCount = foldTraversals(Map.empty[String, Int])({ (acc, sub) =>
-      val labels = extract({
-        case As(label) :: _ => label
-      })(sub)
-      labels.foldLeft(acc) { (acc, label) =>
-        acc + ((label, acc.getOrElse(label, 0) + 1))
-      }
+    val aliasCount = countInTraversals({
+      case As(label) :: _ => label
     })(steps)
 
     val replaceAliases = foldTraversals(Map.empty[String, String])({ (acc, sub) =>

translation/src/main/scala/org/opencypher/gremlin/translation/ir/rewrite/RemoveUnusedAliases.scala

@@ -73,6 +73,7 @@ object RemoveUnusedAliases extends GremlinRewriter {
       case StartsWith(value)      => strings(value)
       case EndsWith(value)        => strings(value)
       case Contains(value)        => strings(value)
+      case IsNode()               => Seq()
     }
   }
 }

translation/src/main/scala/org/opencypher/gremlin/translation/ir/rewrite/SimplifyDelete.scala

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2018 "Neo4j, Inc." [https://neo4j.com]
+ *
+ * Licensed 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.opencypher.gremlin.translation.ir.rewrite
+
+import org.opencypher.gremlin.translation.Tokens.{DELETE, DETACH_DELETE, NULL}
+import org.opencypher.gremlin.translation.ir.TraversalHelper._
+import org.opencypher.gremlin.translation.ir.model._
+
+/**
+  * Removes surplus actions if `delete` or `detach delete` is not used in query
+  */
+object SimplifyDelete extends GremlinRewriter {
+  def apply(steps: Seq[GremlinStep]): Seq[GremlinStep] = {
+    val withChoose = foldTraversals(false)((acc, localSteps) => {
+      acc || extract({
+        case ChooseP(IsNode(), Aggregate(DELETE) :: Nil, Aggregate(DETACH_DELETE) :: Nil) :: _ => true
+      })(localSteps).contains(true)
+    })(steps)
+
+    if (withChoose) {
+      return steps
+    }
+
+    val aggregations = countInTraversals({
+      case Aggregate(sideEffectKey) :: _ => sideEffectKey
+    })(steps)
+
+    val delete = aggregations.getOrElse(DELETE, 0) > 1
+    val detachDelete = aggregations.getOrElse(DETACH_DELETE, 0) > 1
+
+    mapTraversals(
+      replace({
+          case SideEffect(Limit(0) :: Aggregate(DELETE) :: Nil) :: SideEffect(
+                Limit(0) :: Aggregate(DETACH_DELETE) :: Nil) :: rest =>
+            rest
+          case Cap(DELETE) :: Unfold :: Dedup() :: Is(Neq(NULL)) :: SideEffect(_) :: Drop :: rest if !delete =>
+            rest
+          case Cap(DETACH_DELETE) :: Unfold :: Dedup() :: Is(Neq(NULL)) :: Drop :: rest if !detachDelete =>
+            rest
+        }))(steps)
+  }
+}

translation/src/main/scala/org/opencypher/gremlin/translation/translator/TranslatorFlavor.scala

@@ -49,6 +49,7 @@ object TranslatorFlavor {
       RemoveIdentityReselect,
       RemoveUnusedAliases,
       SimplifyEdgeTraversal,
+      SimplifyDelete,
       RemoveUnusedAliases,
       RemoveUselessSteps
     ),