CNDB-15280: Fix AbstractReadQuery.toCQLString to produce correct CQL when possible

src/java/org/apache/cassandra/db/Clustering.java

@@ -49,7 +49,7 @@ public default Clustering<?> clone(ByteBufferCloner cloner)
         ByteBuffer[] newValues = new ByteBuffer[size()];
         for (int i = 0; i < size(); i++)
         {
-            ByteBuffer val = accessor().toBuffer(get(i));
+            ByteBuffer val = bufferAt(i);
             newValues[i] = val == null ? null : cloner.clone(val);
         }
         return new BufferClustering(newValues);
@@ -84,7 +84,8 @@ public default String toCQLString(TableMetadata metadata)
         for (int i = 0; i < size(); i++)
         {
             ColumnMetadata c = metadata.clusteringColumns().get(i);
-            sb.append(i == 0 ? "" : ", ").append(c.type.getString(get(i), accessor()));
+            ByteBuffer value = bufferAt(i);
+            sb.append(i == 0 ? "" : ", ").append(c.type.toCQLString(value));
         }
         return sb.toString();
     }

src/java/org/apache/cassandra/db/ClusteringPrefix.java

@@ -358,11 +358,11 @@ default int dataSize()
     default ByteBuffer serializeAsPartitionKey()
     {
         if (size() == 1)
-            return accessor().toBuffer(get(0));
+            return bufferAt(0);
 
         ByteBuffer[] values = new ByteBuffer[size()];
         for (int i = 0; i < size(); i++)
-            values[i] = accessor().toBuffer(get(i));
+            values[i] = bufferAt(i);
         return CompositeType.build(ByteBufferAccessor.instance, values);
     }
 
@@ -752,4 +752,4 @@ public static boolean equals(ClusteringPrefix<?> prefix, Object o)
         return equals(prefix, (ClusteringPrefix<?>) o);
     }
 
-}
+}

src/java/org/apache/cassandra/db/DataRange.java

@@ -19,7 +19,6 @@
 import java.io.IOException;
 import java.nio.ByteBuffer;
 
-import org.apache.cassandra.cql3.CqlBuilder;
 import org.apache.cassandra.schema.ColumnMetadata;
 import org.apache.cassandra.schema.TableMetadata;
 import org.apache.cassandra.db.filter.*;
@@ -277,54 +276,47 @@ public String toCQLString(TableMetadata metadata)
         if (isUnrestricted())
             return "";
 
-        CqlBuilder builder = new CqlBuilder();
-
-        boolean needAnd = false;
-
         if (isSinglePartition())
         {
             /*
              * Single partition queries using an index are internally mapped to range commands where the start and end
              * key are the same. If that is the case, we want to print the query as an equality on the partition key
              * rather than a token range, as if it was a partition query, for better readability.
              */
-            builder.append(((DecoratedKey) startKey()).toCQLString(metadata));
-            needAnd = true;
+            return ((DecoratedKey) startKey()).toCQLString(metadata);
         }
         else
         {
+            StringBuilder builder = new StringBuilder();
             if (!startKey().isMinimum())
             {
-                appendClause(startKey(), builder, metadata, true, keyRange.isStartInclusive());
-                needAnd = true;
+                appendCQLClause(startKey(), builder, metadata, true, keyRange.isStartInclusive());
             }
             if (!stopKey().isMinimum())
             {
-                if (needAnd)
+                if (builder.length() > 0)
                     builder.append(" AND ");
-                appendClause(stopKey(), builder, metadata, false, keyRange.isEndInclusive());
-                needAnd = true;
+                appendCQLClause(stopKey(), builder, metadata, false, keyRange.isEndInclusive());
             }
+            return builder.toString();
         }
-
-        String filterString = clusteringIndexFilter.toCQLString(metadata);
-        if (!filterString.isEmpty())
-            builder.append(needAnd ? " AND " : "").append(filterString);
-
-        return builder.toString();
     }
 
-    private void appendClause(PartitionPosition pos, CqlBuilder builder, TableMetadata metadata, boolean isStart, boolean isInclusive)
+    private void appendCQLClause(PartitionPosition pos,
+                                 StringBuilder builder,
+                                 TableMetadata metadata,
+                                 boolean isStart,
+                                 boolean isInclusive)
     {
         builder.append("token(");
         builder.append(ColumnMetadata.toCQLString(metadata.partitionKeyColumns()));
         builder.append(") ");
         if (pos instanceof DecoratedKey)
         {
-            builder.append(getOperator(isStart, isInclusive)).append(" ");
+            builder.append(getOperator(isStart, isInclusive)).append(' ');
             builder.append("token(");
             appendKeyString(builder, metadata.partitionKeyType, ((DecoratedKey)pos).getKey());
-            builder.append(")");
+            builder.append(')');
         }
         else
         {
@@ -343,18 +335,18 @@ private static String getOperator(boolean isStart, boolean isInclusive)
 
     // TODO: this is reused in SinglePartitionReadCommand but this should not really be here. Ideally
     // we need a more "native" handling of composite partition keys.
-    public static void appendKeyString(CqlBuilder builder, AbstractType<?> type, ByteBuffer key)
+    public static void appendKeyString(StringBuilder builder, AbstractType<?> type, ByteBuffer key)
     {
         if (type instanceof CompositeType)
         {
             CompositeType ct = (CompositeType)type;
             ByteBuffer[] values = ct.split(key);
             for (int i = 0; i < ct.subTypes().size(); i++)
-                builder.append(i == 0 ? "" : ", ").append(ct.subTypes().get(i).getString(values[i]));
+                builder.append(i == 0 ? "" : ", ").append(ct.subTypes().get(i).toCQLString(values[i]));
         }
         else
         {
-            builder.append(type.getString(key));
+            builder.append(type.toCQLString(key));
         }
     }
 

src/java/org/apache/cassandra/db/DecoratedKey.java

@@ -170,6 +170,8 @@ public String toString()
      * Generate CQL representation of this partition key for the given table.
      * For single-column keys: "k = 0"
      * For multi-column keys: "k1 = 1 AND k2 = 2"
+     *
+     * @param metadata the table metadata
      */
     public String toCQLString(TableMetadata metadata)
     {
@@ -189,7 +191,7 @@ public String toCQLString(TableMetadata metadata)
 
     private static String toCQLString(ColumnMetadata metadata, ByteBuffer key)
     {
-        return String.format("%s = %s", metadata.name.toCQLString(), metadata.type.getString(key));
+        return String.format("%s = %s", metadata.name.toCQLString(), metadata.type.toCQLString(key));
     }
 
     public Token getToken()

src/java/org/apache/cassandra/db/MultiPartitionReadQuery.java

@@ -0,0 +1,81 @@
+/*
+ * Copyright DataStax, Inc.
+ *
+ * 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.apache.cassandra.db;
+
+import java.util.List;
+
+import org.apache.cassandra.cql3.CqlBuilder;
+import org.apache.cassandra.schema.TableMetadata;
+
+/**
+ * A {@code ReadQuery} for multiple partitions, restricted by one of more data ranges.
+ */
+public interface MultiPartitionReadQuery extends ReadQuery
+{
+    List<DataRange> ranges();
+
+    default void appendCQLWhereClause(CqlBuilder builder)
+    {
+        // Append the data ranges.
+        TableMetadata metadata = metadata();
+        boolean hasRanges = appendRanges(builder);
+
+        // Append the clustering index filter and the row filter.
+        String filter = ranges().get(0).clusteringIndexFilter.toCQLString(metadata, rowFilter());
+        if (!filter.isEmpty())
+        {
+            if (filter.startsWith("ORDER BY"))
+                builder.append(" ");
+            else if (hasRanges)
+                builder.append(" AND ");
+            else
+                builder.append(" WHERE ");
+            builder.append(filter);
+        }
+    }
+
+    private boolean appendRanges(CqlBuilder builder)
+    {
+        List<DataRange> ranges = ranges();
+        if (ranges.size() == 1)
+        {
+            DataRange range = ranges.get(0);
+            if (range.isUnrestricted())
+                return false;
+
+            String rangeString = range.toCQLString(metadata());
+            if (!rangeString.isEmpty())
+            {
+                builder.append(" WHERE ").append(rangeString);
+                return true;
+            }
+        }
+        else
+        {
+            builder.append(" WHERE ").append('(');
+            for (int i = 0; i < ranges.size(); i++)
+            {
+                if (i > 0)
+                    builder.append(" OR ");
+                builder.append(ranges.get(i).toCQLString(metadata()));
+            }
+            builder.append(')');
+            return true;
+        }
+        return false;
+    }
+}

src/java/org/apache/cassandra/db/MultiRangeReadCommand.java

@@ -56,7 +56,7 @@
  * Note: digest is not supported because each replica is responsible for different token ranges, there is no point on
  * sending digest.
  */
-public class MultiRangeReadCommand extends ReadCommand
+public class MultiRangeReadCommand extends ReadCommand implements MultiPartitionReadQuery
 {
     protected static final SelectionDeserializer selectionDeserializer = new Deserializer();
 
@@ -142,6 +142,7 @@ public boolean isSinglePartition()
     /**
      * @return all token ranges to be queried
      */
+    @Override
     public List<DataRange> ranges()
     {
         return dataRanges;
@@ -320,31 +321,9 @@ public Verb verb()
     }
 
     @Override
-    protected void appendCQLWhereClause(CqlBuilder builder)
+    public void appendCQLWhereClause(CqlBuilder builder)
     {
-        if (ranges().size() == 1 && ranges().get(0).isUnrestricted() && rowFilter().isEmpty())
-            return;
-
-        builder.append(" WHERE ");
-        // We put the row filter first because the data range can end by "ORDER BY"
-        if (!rowFilter().isEmpty())
-        {
-            builder.append(rowFilter());
-            builder.append(" AND ");
-        }
-
-        boolean isFirst = true;
-        for (int i = 0; i < ranges().size(); i++)
-        {
-            DataRange dataRange = ranges().get(i);
-            if (!dataRange.isUnrestricted())
-            {
-                if (!isFirst)
-                    builder.append(" AND ");
-                isFirst = false;
-                builder.append(dataRange.toCQLString(metadata()));
-            }
-        }
+        MultiPartitionReadQuery.super.appendCQLWhereClause(builder);
     }
 
     @Override

src/java/org/apache/cassandra/db/PartitionRangeReadCommand.java

@@ -394,23 +394,9 @@ public Verb verb()
     }
 
     @Override
-    protected void appendCQLWhereClause(CqlBuilder builder)
+    public void appendCQLWhereClause(CqlBuilder builder)
     {
-        if (dataRange.isUnrestricted() && rowFilter().isEmpty())
-            return;
-
-        builder.append(" WHERE ");
-        // We put the row filter first because the data range can end by "ORDER BY"
-        if (!rowFilter().isEmpty())
-        {
-            builder.append(rowFilter());
-            if (!dataRange.isUnrestricted())
-                builder.append(" AND ");
-        }
-        if (!dataRange.isUnrestricted())
-        {
-            builder.append(dataRange.toCQLString(metadata()));
-        }
+        PartitionRangeReadQuery.super.appendCQLWhereClause(builder);
     }
 
     @Override

src/java/org/apache/cassandra/db/PartitionRangeReadQuery.java

@@ -17,6 +17,8 @@
  */
 package org.apache.cassandra.db;
 
+import java.util.List;
+
 import org.apache.cassandra.db.filter.ColumnFilter;
 import org.apache.cassandra.db.filter.DataLimits;
 import org.apache.cassandra.db.filter.RowFilter;
@@ -29,7 +31,7 @@
 /**
  *  A {@code ReadQuery} for a range of partitions.
  */
-public interface PartitionRangeReadQuery extends ReadQuery
+public interface PartitionRangeReadQuery extends MultiPartitionReadQuery
 {
     static ReadQuery create(TableMetadata table,
                             int nowInSec,
@@ -46,6 +48,12 @@ static ReadQuery create(TableMetadata table,
 
     DataRange dataRange();
 
+    @Override
+    default List<DataRange> ranges()
+    {
+        return List.of(dataRange());
+    }
+
     /**
      * Creates a new {@code PartitionRangeReadQuery} with the updated limits.
      *

src/java/org/apache/cassandra/db/ReadCommand.java

@@ -37,7 +37,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import org.apache.cassandra.cql3.CqlBuilder;
 import org.apache.cassandra.db.filter.ClusteringIndexFilter;
 import org.apache.cassandra.db.filter.ColumnFilter;
 import org.apache.cassandra.db.filter.DataLimits;
@@ -762,8 +761,6 @@ public Message<ReadCommand> createMessage(boolean trackRepairedData)
 
     public abstract Verb verb();
 
-    protected abstract void appendCQLWhereClause(CqlBuilder builder);
-
     // Skip purgeable tombstones. We do this because it's safe to do (post-merge of the memtable and sstable at least), it
     // can save us some bandwith, and avoid making us throw a TombstoneOverwhelmingException for purgeable tombstones (which
     // are to some extend an artefact of compaction lagging behind and hence counting them is somewhat unintuitive).

src/java/org/apache/cassandra/db/SinglePartitionReadCommand.java

@@ -41,7 +41,6 @@
 import org.apache.cassandra.db.transform.Transformation;
 import org.apache.cassandra.exceptions.RequestExecutionException;
 import org.apache.cassandra.index.Index;
-import org.apache.cassandra.index.sai.utils.RowWithSourceTable;
 import org.apache.cassandra.io.sstable.format.RowIndexEntry;
 import org.apache.cassandra.io.sstable.format.SSTableReader;
 import org.apache.cassandra.io.sstable.format.SSTableReadsListener;
@@ -1153,19 +1152,9 @@ public Verb verb()
     }
 
     @Override
-    protected void appendCQLWhereClause(CqlBuilder builder)
+    public void appendCQLWhereClause(CqlBuilder builder)
     {
-        builder.append(" WHERE ");
-
-        builder.append(partitionKey().toCQLString(metadata()));
-
-        // We put the row filter first because the clustering index filter can end by "ORDER BY"
-        if (!rowFilter().isEmpty())
-            builder.append(" AND ").append(rowFilter());
-
-        String filterString = clusteringIndexFilter().toCQLString(metadata());
-        if (!filterString.isEmpty())
-            builder.append(" AND ").append(filterString);
+        SinglePartitionReadQuery.super.appendCQLWhereClause(builder);
     }
 
     protected void serializeSelection(DataOutputPlus out, int version) throws IOException