Fix static column order for SELECT * wildcard queries

patch by Aleksey Yeschenko; reviewed by Benedict Elliott Smith for
CASSANDRA-14638

CHANGES.txt

@@ -1,4 +1,5 @@
 3.0.18
+ * Fix static column order for SELECT * wildcard queries (CASSANDRA-14638)
  * sstableloader should use discovered broadcast address to connect intra-cluster (CASSANDRA-14522)
  * Fix reading columns with non-UTF names from schema (CASSANDRA-14468)
  Merged from 2.2:

NEWS.txt

@@ -42,6 +42,16 @@ restore snapshots created with the previous major version using the
 'sstableloader' tool. You can upgrade the file format of your snapshots
 using the provided 'sstableupgrade' tool.
 
+3.0.18
+======
+
+Upgrading
+---------
+    - The order of static columns in SELECT * has been fixed to match that of 2.0 and 2.1 - they are now sorted
+      alphabetically again, by their name, just like regular columns are. If you use prepared statements and
+      SELECT * queries, and have both simple and collection static columns in those tables, and are upgrading from an
+      earlier 3.0 version, then you might be affected by this change. Please see CASSANDRA-14638 for details.
+
 3.0.17
 =====
 

doc/cql3/CQL.textile

@@ -1079,7 +1079,7 @@ The @SELECT@ statements reads one or more columns for one or more rows in a tabl
 
 h4(#selectSelection). @<select-clause>@
 
-The @<select-clause>@ determines which columns needs to be queried and returned in the result-set. It consists of either the comma-separated list of <selector> or the wildcard character (@*@) to select all the columns defined for the table.
+The @<select-clause>@ determines which columns needs to be queried and returned in the result-set. It consists of either the comma-separated list of <selector> or the wildcard character (@*@) to select all the columns defined for the table. Please note that for wildcard @SELECT@ queries the order of columns returned is not specified and is not guaranteed to be stable between Cassandra versions.
 
 A @<selector>@ is either a column name to retrieve or a @<function>@ of one or more @<term>@s. The function allowed are the same as for @<term>@ and are described in the "function section":#functions. In addition to these generic functions, the @WRITETIME@ (resp. @TTL@) function allows to select the timestamp of when the column was inserted (resp. the time to live (in seconds) for the column (or null if the column has no expiration set)).
 

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

@@ -50,7 +50,16 @@ public class Columns extends AbstractCollection<ColumnDefinition> implements Col
 {
     public static final Serializer serializer = new Serializer();
     public static final Columns NONE = new Columns(BTree.empty(), 0);
-    public static final ColumnDefinition FIRST_COMPLEX =
+
+    private static final ColumnDefinition FIRST_COMPLEX_STATIC =
+        new ColumnDefinition("",
+                             "",
+                             ColumnIdentifier.getInterned(ByteBufferUtil.EMPTY_BYTE_BUFFER, UTF8Type.instance),
+                             SetType.getInstance(UTF8Type.instance, true),
+                             ColumnDefinition.NO_POSITION,
+                             ColumnDefinition.Kind.STATIC);
+
+    private static final ColumnDefinition FIRST_COMPLEX_REGULAR =
         new ColumnDefinition("",
                              "",
                              ColumnIdentifier.getInterned(ByteBufferUtil.EMPTY_BYTE_BUFFER, UTF8Type.instance),
@@ -99,11 +108,14 @@ public class Columns extends AbstractCollection<ColumnDefinition> implements Col
 
     private static int findFirstComplexIdx(Object[] tree)
     {
-        // have fast path for common no-complex case
+        if (BTree.isEmpty(tree))
+            return 0;
+
         int size = BTree.size(tree);
-        if (!BTree.isEmpty(tree) && BTree.<ColumnDefinition>findByIndex(tree, size - 1).isSimple())
-            return size;
-        return BTree.ceilIndex(tree, Comparator.naturalOrder(), FIRST_COMPLEX);
+        ColumnDefinition last = BTree.findByIndex(tree, size - 1);
+        return last.isSimple()
+             ? size
+             : BTree.ceilIndex(tree, Comparator.naturalOrder(), last.isStatic() ? FIRST_COMPLEX_STATIC : FIRST_COMPLEX_REGULAR);
     }
 
     /**

test/unit/org/apache/cassandra/db/ColumnsTest.java

@@ -23,6 +23,7 @@ import java.util.*;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.function.Predicate;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Lists;
 
@@ -33,6 +34,7 @@ import junit.framework.Assert;
 import org.apache.cassandra.MockSchema;
 import org.apache.cassandra.config.CFMetaData;
 import org.apache.cassandra.config.ColumnDefinition;
+import org.apache.cassandra.db.marshal.AbstractType;
 import org.apache.cassandra.db.marshal.SetType;
 import org.apache.cassandra.db.marshal.UTF8Type;
 import org.apache.cassandra.io.util.DataInputBuffer;
@@ -129,21 +131,77 @@ public class ColumnsTest
     {
         List<String> names = new ArrayList<>();
         for (int i = 0; i < 50; i++)
-            names.add("clustering_" + i);
+            names.add("regular_" + i);
 
         List<ColumnDefinition> defs = new ArrayList<>();
-        addClustering(names, defs);
+        addRegular(names, defs);
 
         Columns columns = Columns.from(new HashSet<>(defs));
 
         defs = new ArrayList<>();
-        addClustering(names.subList(0, 8), defs);
+        addRegular(names.subList(0, 8), defs);
 
         Columns subset = Columns.from(new HashSet<>(defs));
 
         Assert.assertTrue(columns.containsAll(subset));
     }
 
+    @Test
+    public void testStaticColumns()
+    {
+        testColumns(ColumnDefinition.Kind.STATIC);
+    }
+
+    @Test
+    public void testRegularColumns()
+    {
+        testColumns(ColumnDefinition.Kind.REGULAR);
+    }
+
+    private void testColumns(ColumnDefinition.Kind kind)
+    {
+        List<ColumnDefinition> definitions = ImmutableList.of(
+            def("a", UTF8Type.instance, kind),
+            def("b", SetType.getInstance(UTF8Type.instance, true), kind),
+            def("c", UTF8Type.instance, kind),
+            def("d", SetType.getInstance(UTF8Type.instance, true), kind),
+            def("e", UTF8Type.instance, kind),
+            def("f", SetType.getInstance(UTF8Type.instance, true), kind),
+            def("g", UTF8Type.instance, kind),
+            def("h", SetType.getInstance(UTF8Type.instance, true), kind)
+        );
+        Columns columns = Columns.from(definitions);
+
+        // test simpleColumnCount()
+        Assert.assertEquals(4, columns.simpleColumnCount());
+
+        // test simpleColumns()
+        List<ColumnDefinition> simpleColumnsExpected =
+            ImmutableList.of(definitions.get(0), definitions.get(2), definitions.get(4), definitions.get(6));
+        List<ColumnDefinition> simpleColumnsActual = new ArrayList<>();
+        Iterators.addAll(simpleColumnsActual, columns.simpleColumns());
+        Assert.assertEquals(simpleColumnsExpected, simpleColumnsActual);
+
+        // test complexColumnCount()
+        Assert.assertEquals(4, columns.complexColumnCount());
+
+        // test complexColumns()
+        List<ColumnDefinition> complexColumnsExpected =
+            ImmutableList.of(definitions.get(1), definitions.get(3), definitions.get(5), definitions.get(7));
+        List<ColumnDefinition> complexColumnsActual = new ArrayList<>();
+        Iterators.addAll(complexColumnsActual, columns.complexColumns());
+        Assert.assertEquals(complexColumnsExpected, complexColumnsActual);
+
+        // test size()
+        Assert.assertEquals(8, columns.size());
+
+        // test selectOrderIterator()
+        List<ColumnDefinition> columnsExpected = definitions;
+        List<ColumnDefinition> columnsActual = new ArrayList<>();
+        Iterators.addAll(columnsActual, columns.selectOrderIterator());
+        Assert.assertEquals(columnsExpected, columnsActual);
+    }
+
     private void testSerializeSubset(ColumnsCheck input) throws IOException
     {
         testSerializeSubset(input.columns, input.columns, input.definitions);
@@ -403,6 +461,11 @@ public class ColumnsTest
             results.add(ColumnDefinition.regularDef(cfMetaData, bytes(name), SetType.getInstance(UTF8Type.instance, true)));
     }
 
+    private static ColumnDefinition def(String name, AbstractType<?> type, ColumnDefinition.Kind kind)
+    {
+        return new ColumnDefinition(cfMetaData, bytes(name), type, ColumnDefinition.NO_POSITION, kind);
+    }
+
     private static CFMetaData mock(Columns columns)
     {
         if (columns.isEmpty())