apache · adutra · Jan 8, 2020 · Dec 23, 2019 · Dec 23, 2019 · Dec 30, 2019
diff --git a/changelog/README.md b/changelog/README.md
@@ -4,6 +4,8 @@
 
 ### 4.4.0 (in progress)
 
+- [improvement] JAVA-2596: Consider collection removals as idempotent in query builder
+- [bug] JAVA-2555: Generate append/prepend constructs compatible with legacy C* versions
 - [bug] JAVA-2584: Ensure codec registry is able to create codecs for collections of UDTs and tuples
 - [bug] JAVA-2583: IS NOT NULL clause should be idempotent
 - [improvement] JAVA-2442: Don't check for schema agreement twice when completing a DDL query

diff --git a/manual/query_builder/idempotence/README.md b/manual/query_builder/idempotence/README.md
@@ -39,15 +39,15 @@ If you use the result of a user-defined function in an INSERT or UPDATE statemen
 of knowing if that function is idempotent:  
 
 ```java
-Statement statement = insertInto("foo").value("k", function("generate_id")).build();
+SimpleStatement statement = insertInto("foo").value("k", function("generate_id")).build();
 // INSERT INTO foo (k) VALUES (generate_id())
 assert !statement.isIdempotent();
 ```
 
 This extends to arithmetic operations using such terms:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     insertInto("foo").value("k", add(function("generate_id"), literal(1))).build();
 // INSERT INTO foo (k) VALUES (generate_id()+1)
 assert !statement.isIdempotent();
@@ -56,7 +56,7 @@ assert !statement.isIdempotent();
 Raw terms could be anything, so they are also considered unsafe by default: 
 
 ```java
-Statement statement =
+SimpleStatement statement =
     insertInto("foo").value("k", raw("generate_id()+1")).build();
 // INSERT INTO foo (k) VALUES (generate_id()+1)
 assert !statement.isIdempotent();
@@ -68,7 +68,7 @@ If a WHERE clause in an UPDATE or DELETE statement uses a comparison with an uns
 potentially apply to different rows for each execution:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     update("foo")
         .setColumn("v", bindMarker())
         .whereColumn("k").isEqualTo(function("non_idempotent_func"))
@@ -82,7 +82,7 @@ assert !statement.isIdempotent();
 Counter updates are never idempotent:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     update("foo")
         .increment("c")
         .whereColumn("k").isEqualTo(bindMarker())
@@ -94,20 +94,20 @@ assert !statement.isIdempotent();
 Nor is appending or prepending an element to a list:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     update("foo")
         .appendListElement("l", literal(1))
         .whereColumn("k").isEqualTo(bindMarker())
         .build();
-// UPDATE foo SET l+=[1] WHERE k=?
+// UPDATE foo SET l=l+[1] WHERE k=?
 assert !statement.isIdempotent();
 ```
 
 The generic `append` and `prepend` methods apply to any kind of collection, so we have to consider
 them unsafe by default too:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     update("foo")
         .prepend("l", literal(Arrays.asList(1, 2, 3)))
         .whereColumn("k").isEqualTo(bindMarker())
@@ -116,12 +116,66 @@ Statement statement =
 assert !statement.isIdempotent();
 ```
 
+The generic `remove` method is however safe since collection removals are idempotent:
+
+```java
+SimpleStatement statement =
+    update("foo")
+        .remove("l", literal(Arrays.asList(1, 2, 3)))
+        .whereColumn("k").isEqualTo(bindMarker())
+        .build();
+// UPDATE foo SET l=l-[1,2,3] WHERE k=?
+assert statement.isIdempotent();
+```
+
+When appending, prepending or removing a single element to/from a collection, it is possible to use 
+the dedicated methods listed below; their idempotence depends on the collection type (list, set or 
+map), the operation (append, prepend or removal) and the idempotence of the element being 
+added/removed:
+
+1. `appendListElement` : not idempotent
+2. `prependListElement` : not idempotent
+3. `removeListElement` : idempotent if element is idempotent
+4. `appendSetElement` : idempotent if element is idempotent
+5. `prependSetElement` : idempotent if element is idempotent
+6. `removeSetElement` : idempotent if element is idempotent
+7. `appendMapElement` : idempotent if both key and value are idempotent
+8. `prependMapElement` : idempotent if both key and value are idempotent
+9. `removeMapElement` : idempotent if both key and value are idempotent
+
+In practice, most invocations of the above methods will be idempotent because most collection 
+elements are. For example, the following statement is idempotent since `literal(1)` is also 
+idempotent:
+
+```java
+SimpleStatement statement =
+    update("foo")
+        .removeListElement("l", literal(1))
+        .whereColumn("k").isEqualTo(bindMarker())
+        .build();
+// UPDATE foo SET l=l-[1] WHERE k=?
+assert statement.isIdempotent();
+```
+
+However, in rare cases the resulting statement won't be marked idempotent, e.g. if you use a 
+function to select a collection element:
+
+```java
+SimpleStatement statement =
+    update("foo")
+        .removeListElement("l", function("myfunc"))
+        .whereColumn("k").isEqualTo(bindMarker())
+        .build();
+// UPDATE foo SET l=l-[myfunc()] WHERE k=?
+assert !statement.isIdempotent();
+```
+
 ### Unsafe deletions
 
 Deleting from a list is not idempotent:
 
 ```java
-Statement statement =
+SimpleStatement statement =
     deleteFrom("foo")
         .element("l", literal(0))
         .whereColumn("k").isEqualTo(bindMarker())

diff --git a/query-builder/src/main/java/com/datastax/oss/driver/api/querybuilder/update/Assignment.java b/query-builder/src/main/java/com/datastax/oss/driver/api/querybuilder/update/Assignment.java
@@ -34,6 +34,7 @@
 import com.datastax.oss.driver.internal.querybuilder.update.PrependListElementAssignment;
 import com.datastax.oss.driver.internal.querybuilder.update.PrependMapEntryAssignment;
 import com.datastax.oss.driver.internal.querybuilder.update.PrependSetElementAssignment;
+import com.datastax.oss.driver.internal.querybuilder.update.RemoveAssignment;
 import com.datastax.oss.driver.internal.querybuilder.update.RemoveListElementAssignment;
 import com.datastax.oss.driver.internal.querybuilder.update.RemoveMapEntryAssignment;
 import com.datastax.oss.driver.internal.querybuilder.update.RemoveSetElementAssignment;
@@ -146,13 +147,13 @@ static Assignment decrement(@NonNull String columnName) {
   }
 
   /**
-   * Appends to a collection column, as in {@code SET l+=?}.
+   * Appends to a collection column, as in {@code SET l=l+?}.
    *
    * <p>The term must be a collection of the same type as the column.
    */
   @NonNull
   static Assignment append(@NonNull CqlIdentifier columnId, @NonNull Term suffix) {
-    return new AppendAssignment(new ColumnLeftOperand(columnId), suffix);
+    return new AppendAssignment(columnId, suffix);
   }
 
   /**
@@ -165,7 +166,7 @@ static Assignment append(@NonNull String columnName, @NonNull Term suffix) {
   }
 
   /**
-   * Appends a single element to a list column, as in {@code SET l+=[?]}.
+   * Appends a single element to a list column, as in {@code SET l=l+[?]}.
    *
    * <p>The term must be of the same type as the column's elements.
    */
@@ -184,7 +185,7 @@ static Assignment appendListElement(@NonNull String columnName, @NonNull Term su
   }
 
   /**
-   * Appends a single element to a set column, as in {@code SET s+={?}}.
+   * Appends a single element to a set column, as in {@code SET s=s+{?}}.
    *
    * <p>The term must be of the same type as the column's elements.
    */
@@ -203,7 +204,7 @@ static Assignment appendSetElement(@NonNull String columnName, @NonNull Term suf
   }
 
   /**
-   * Appends a single entry to a map column, as in {@code SET m+={?:?}}.
+   * Appends a single entry to a map column, as in {@code SET m=m+{?:?}}.
    *
    * <p>The terms must be of the same type as the column's keys and values respectively.
    */
@@ -302,7 +303,7 @@ static Assignment prependMapEntry(
   }
 
   /**
-   * Removes elements from a collection, as in {@code SET l-=[1,2,3]}.
+   * Removes elements from a collection, as in {@code SET l=l-[1,2,3]}.
    *
    * <p>The term must be a collection of the same type as the column.
    *
@@ -313,7 +314,7 @@ static Assignment prependMapEntry(
    */
   @NonNull
   static Assignment remove(@NonNull CqlIdentifier columnId, @NonNull Term collectionToRemove) {
-    return new DefaultAssignment(new ColumnLeftOperand(columnId), "-=", collectionToRemove);
+    return new RemoveAssignment(columnId, collectionToRemove);
   }
 
   /**
@@ -326,7 +327,7 @@ static Assignment remove(@NonNull String columnName, @NonNull Term collectionToR
   }
 
   /**
-   * Removes a single element to a list column, as in {@code SET l-=[?]}.
+   * Removes a single element from a list column, as in {@code SET l=l-[?]}.
    *
    * <p>The term must be of the same type as the column's elements.
    */
@@ -345,7 +346,7 @@ static Assignment removeListElement(@NonNull String columnName, @NonNull Term su
   }
 
   /**
-   * Removes a single element to a set column, as in {@code SET s-={?}}.
+   * Removes a single element from a set column, as in {@code SET s=s-{?}}.
    *
    * <p>The term must be of the same type as the column's elements.
    */
@@ -364,7 +365,7 @@ static Assignment removeSetElement(@NonNull String columnName, @NonNull Term suf
   }
 
   /**
-   * Removes a single entry to a map column, as in {@code SET m-={?:?}}.
+   * Removes a single entry from a map column, as in {@code SET m=m-{?:?}}.
    *
    * <p>The terms must be of the same type as the column's keys and values respectively.
    */

diff --git a/...lder/src/main/java/com/datastax/oss/driver/api/querybuilder/update/OngoingAssignment.java b/...lder/src/main/java/com/datastax/oss/driver/api/querybuilder/update/OngoingAssignment.java
@@ -221,7 +221,7 @@ default UpdateWithAssignments decrement(@NonNull String columnName) {
   }
 
   /**
-   * Appends to a collection column, as in {@code SET l+=?}.
+   * Appends to a collection column, as in {@code SET l=l+?}.
    *
    * <p>The term must be a collection of the same type as the column.
    *
@@ -246,7 +246,7 @@ default UpdateWithAssignments append(@NonNull String columnName, @NonNull Term s
   }
 
   /**
-   * Appends a single element to a list column, as in {@code SET l+=[?]}.
+   * Appends a single element to a list column, as in {@code SET l=l+[?]}.
    *
    * <p>The term must be of the same type as the column's elements.
    *
@@ -274,7 +274,7 @@ default UpdateWithAssignments appendListElement(
   }
 
   /**
-   * Appends a single element to a set column, as in {@code SET s+={?}}.
+   * Appends a single element to a set column, as in {@code SET s=s+{?}}.
    *
    * <p>The term must be of the same type as the column's elements.
    *
@@ -299,7 +299,7 @@ default UpdateWithAssignments appendSetElement(@NonNull String columnName, @NonN
   }
 
   /**
-   * Appends a single entry to a map column, as in {@code SET m+={?:?}}.
+   * Appends a single entry to a map column, as in {@code SET m=m+{?:?}}.
    *
    * <p>The terms must be of the same type as the column's keys and values respectively.
    *
@@ -436,7 +436,7 @@ default UpdateWithAssignments prependMapEntry(
   }
 
   /**
-   * Removes elements from a collection, as in {@code SET l-=[1,2,3]}.
+   * Removes elements from a collection, as in {@code SET l=l-[1,2,3]}.
    *
    * <p>The term must be a collection of the same type as the column.
    *
@@ -469,7 +469,7 @@ default UpdateWithAssignments remove(
   }
 
   /**
-   * Removes a single element to a list column, as in {@code SET l-=[?]}.
+   * Removes a single element to a list column, as in {@code SET l=l-[?]}.
    *
    * <p>The term must be of the same type as the column's elements.
    *
@@ -497,7 +497,7 @@ default UpdateWithAssignments removeListElement(
   }
 
   /**
-   * Removes a single element to a set column, as in {@code SET s-={?}}.
+   * Removes a single element to a set column, as in {@code SET s=s-{?}}.
    *
    * <p>The term must be of the same type as the column's elements.
    *
@@ -522,7 +522,7 @@ default UpdateWithAssignments removeSetElement(@NonNull String columnName, @NonN
   }
 
   /**
-   * Removes a single entry to a map column, as in {@code SET m-={?:?}}.
+   * Removes a single entry to a map column, as in {@code SET m=m-{?:?}}.
    *
    * <p>The terms must be of the same type as the column's keys and values respectively.
    *

diff --git a/.../src/main/java/com/datastax/oss/driver/internal/querybuilder/update/AppendAssignment.java b/.../src/main/java/com/datastax/oss/driver/internal/querybuilder/update/AppendAssignment.java
@@ -15,21 +15,15 @@
  */
 package com.datastax.oss.driver.internal.querybuilder.update;
 
+import com.datastax.oss.driver.api.core.CqlIdentifier;
 import com.datastax.oss.driver.api.querybuilder.term.Term;
-import com.datastax.oss.driver.internal.querybuilder.lhs.LeftOperand;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import net.jcip.annotations.Immutable;
 
 @Immutable
-public class AppendAssignment extends DefaultAssignment {
+public class AppendAssignment extends CollectionAssignment {
 
-  public AppendAssignment(@NonNull LeftOperand leftOperand, @NonNull Term rightOperand) {
-    super(leftOperand, "+=", rightOperand);
-  }
-
-  @Override
-  public boolean isIdempotent() {
-    // Not idempotent for lists, be pessimistic
-    return false;
+  public AppendAssignment(@NonNull CqlIdentifier columnId, @NonNull Term value) {
+    super(columnId, Operator.APPEND, value);
   }
 }