Refactor the ast package to enable harry model based testing

patch by David Capwell; reviewed by Abe Ratnofsky, Caleb Rackliffe for CASSANDRA-20155

src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java

@@ -388,7 +388,15 @@ public class TransactionStatement implements CQLStatement.CompositeCQLStatement,
 
         // check again since now we have query options; note that statements are quaranted to be single partition reads at this point
         for (NamedSelect assignment : assignments)
+        {
             checkFalse(isSelectingMultipleClusterings(assignment.select, options), INCOMPLETE_PRIMARY_KEY_SELECT_MESSAGE, "LET assignment", assignment.select.source);
+            if (assignment.select.getRestrictions().keyIsInRelation())
+                checkTrue(assignment.select.getLimit(options) == DataLimits.NO_LIMIT, NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", assignment.select.source);
+        }
+        if (returningSelect != null && returningSelect.select.getRestrictions().keyIsInRelation())
+        {
+            checkTrue(returningSelect.select.getLimit(options) == DataLimits.NO_LIMIT, NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", returningSelect.select.source);
+        }
 
         Txn txn = createTxn(state.getClientState(), options);
 
@@ -493,8 +501,9 @@ public class TransactionStatement implements CQLStatement.CompositeCQLStatement,
         if (prepared.hasAggregation())
             throw invalidRequest(NO_AGGREGATION_IN_TXNS_MESSAGE, "SELECT", prepared.source);
 
+        // when "LIMIT ?" this check can't be performed, so need to do again once the options are known
         if (prepared.getRestrictions().keyIsInRelation())
-            checkTrue(prepared.getLimit(null) == DataLimits.NO_LIMIT, NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", prepared.source);
+            checkTrue(prepared.isLimitMarker() || prepared.getLimit(null) == DataLimits.NO_LIMIT, NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", prepared.source);
     }
 
     public static class Parsed extends QualifiedStatement.Composite

test/distributed/org/apache/cassandra/distributed/test/metrics/CoordinatorReadLatencyMetricTest.java

@@ -28,7 +28,6 @@ import org.assertj.core.api.Assertions;
 import org.junit.Test;
 
 import org.apache.cassandra.config.Config;
-import org.apache.cassandra.cql3.ast.Conditional;
 import org.apache.cassandra.cql3.ast.Select;
 import org.apache.cassandra.cql3.ast.Txn;
 import org.apache.cassandra.db.Keyspace;
@@ -40,6 +39,7 @@ import org.apache.cassandra.metrics.ClientRequestsMetricsHolder;
 import org.apache.cassandra.service.consensus.TransactionalMode;
 import org.apache.cassandra.service.paxos.Paxos;
 
+import static org.apache.cassandra.cql3.ast.Conditional.Where.Inequality.LESS_THAN;
 import static org.junit.Assert.assertTrue;
 
 public class CoordinatorReadLatencyMetricTest extends TestBaseImpl
@@ -58,7 +58,7 @@ public class CoordinatorReadLatencyMetricTest extends TestBaseImpl
 //                               .withSelection(FunctionCall.count("v"))
                                .table(KEYSPACE, "tbl")
                                .value("pk", 0)
-                               .where("ck", Conditional.Where.Inequality.LESS_THAN, 42)
+                               .where("ck", LESS_THAN, 42)
                                .limit(1)
                                .build();
 

test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java

@@ -46,8 +46,15 @@ import accord.utils.Property.Command;
 import accord.utils.RandomSource;
 import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.config.Config;
+import org.apache.cassandra.cql3.ast.CQLFormatter;
 import org.apache.cassandra.cql3.ast.Mutation;
+import org.apache.cassandra.cql3.ast.Select;
+import org.apache.cassandra.cql3.ast.StandardVisitors;
 import org.apache.cassandra.cql3.ast.Statement;
+import org.apache.cassandra.cql3.ast.Txn;
+import org.apache.cassandra.db.marshal.AsciiType;
+import org.apache.cassandra.db.marshal.BytesType;
+import org.apache.cassandra.db.marshal.UTF8Type;
 import org.apache.cassandra.distributed.Cluster;
 import org.apache.cassandra.distributed.api.ConsistencyLevel;
 import org.apache.cassandra.distributed.api.IInstanceConfig;
@@ -59,16 +66,27 @@ import org.apache.cassandra.schema.TableMetadata;
 import org.apache.cassandra.service.accord.api.AccordAgent;
 import org.apache.cassandra.service.consensus.TransactionalMode;
 import org.apache.cassandra.utils.ASTGenerators;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
 import org.apache.cassandra.utils.CassandraGenerators;
+import org.apache.cassandra.utils.FastByteOperations;
+import org.apache.cassandra.utils.Generators;
 import org.apache.cassandra.utils.Isolated;
 import org.apache.cassandra.utils.Shared;
+import org.quicktheories.generators.SourceDSL;
 
+import static org.apache.cassandra.utils.AbstractTypeGenerators.overridePrimitiveTypeSupport;
+import static org.apache.cassandra.utils.AbstractTypeGenerators.stringComparator;
 import static org.apache.cassandra.utils.AccordGenerators.fromQT;
 
 public class AccordTopologyMixupTest extends TopologyMixupTestBase<AccordTopologyMixupTest.Spec>
 {
     private static final Logger logger = LoggerFactory.getLogger(AccordTopologyMixupTest.class);
 
+    /**
+     * Should the history show the CQL?  By default, this is off as its very verbose, but when debugging this can be helpful.
+     */
+    private static boolean HISTORY_SHOWS_CQL = false;
+
     static
     {
         CassandraRelevantProperties.ACCORD_AGENT_CLASS.setString(InterceptAgent.class.getName());
@@ -76,6 +94,10 @@ public class AccordTopologyMixupTest extends TopologyMixupTestBase<AccordTopolog
         CassandraRelevantProperties.ACCORD_KEY_PARANOIA_CPU.setString(Invariants.Paranoia.QUADRATIC.name());
         CassandraRelevantProperties.ACCORD_KEY_PARANOIA_MEMORY.setString(Invariants.Paranoia.QUADRATIC.name());
         CassandraRelevantProperties.ACCORD_KEY_PARANOIA_COSTFACTOR.setString(Invariants.ParanoiaCostFactor.HIGH.name());
+
+        overridePrimitiveTypeSupport(AsciiType.instance, AbstractTypeGenerators.TypeSupport.of(AsciiType.instance, SourceDSL.strings().ascii().ofLengthBetween(1, 10), stringComparator(AsciiType.instance)));
+        overridePrimitiveTypeSupport(UTF8Type.instance, AbstractTypeGenerators.TypeSupport.of(UTF8Type.instance, Generators.utf8(1, 10), stringComparator(UTF8Type.instance)));
+        overridePrimitiveTypeSupport(BytesType.instance, AbstractTypeGenerators.TypeSupport.of(BytesType.instance, Generators.bytes(1, 10), FastByteOperations::compareUnsigned));
     }
 
     private static final List<TransactionalMode> TRANSACTIONAL_MODES = Stream.of(TransactionalMode.values()).filter(t -> t.accordIsEnabled).collect(Collectors.toList());
@@ -91,21 +113,27 @@ public class AccordTopologyMixupTest extends TopologyMixupTestBase<AccordTopolog
     {
         // if a failing seed is detected, populate here
         // Example: builder.withSeed(42L);
+        // HISTORY_SHOWS_CQL = true; // uncomment if the CQL done should be included in the history
     }
 
     private static Spec createSchemaSpec(RandomSource rs, Cluster cluster)
     {
         TransactionalMode mode = rs.pick(TRANSACTIONAL_MODES);
         boolean enableMigration = allowsMigration(mode) && rs.nextBoolean();
+        // This test puts a focus on topology / cluster operations, so schema "shouldn't matter"... limit the domain of the test to improve the ability to debug
+        AbstractTypeGenerators.TypeGenBuilder supportedTypes = AbstractTypeGenerators.withoutUnsafeEquality(AbstractTypeGenerators.builder()
+                                                                                                                                  .withTypeKinds(AbstractTypeGenerators.TypeKind.PRIMITIVE));
         TableMetadata metadata = fromQT(new CassandraGenerators.TableMetadataBuilder()
-                .withKeyspaceName(KEYSPACE)
-                .withTableKinds(TableMetadata.Kind.REGULAR)
-                .withKnownMemtables()
-                //TODO (coverage): include "fast_path = 'keyspace'" override
-                .withTransactionalMode(enableMigration ? TransactionalMode.off : mode)
-                .withoutEmpty()
-                .build())
-                .next(rs);
+                                        .withKeyspaceName(KEYSPACE)
+                                        .withTableName("tbl")
+                                        .withTableKinds(TableMetadata.Kind.REGULAR)
+                                        .withKnownMemtables()
+                                        .withSimpleColumnNames()
+                                        //TODO (coverage): include "fast_path = 'keyspace'" override
+                                        .withTransactionalMode(mode)
+                                        .withDefaultTypeGen(supportedTypes)
+                                        .build())
+                                 .next(rs);
         maybeCreateUDTs(cluster, metadata);
         String schemaCQL = metadata.toCqlString(false, false, false);
         logger.info("Creating test table:\n{}", schemaCQL);
@@ -133,14 +161,16 @@ public class AccordTopologyMixupTest extends TopologyMixupTestBase<AccordTopolog
 
     private static Command<State<Spec>, Void, ?> cqlOperation(RandomSource rs, State<Spec> state, Gen<Statement> statementGen)
     {
-        Statement stmt = statementGen.next(rs);
-        String cql;
-        //TODO (usability): are there any transaction_modes that actually need simple mutations/select to be wrapped in a BEGIN TRANSACTION?  If not then this logica can be simplified
-        if (stmt.kind() == Statement.Kind.TXN || stmt.kind() == Statement.Kind.MUTATION && ((Mutation) stmt).isCas())
-            cql = stmt.toCQL();
-        else cql = wrapInTxn(stmt.toCQL());
+        Statement stmt = statementGen.map(s -> {
+            if (s.kind() == Statement.Kind.TXN || s.kind() == Statement.Kind.MUTATION && ((Mutation) s).isCas())
+                return s;
+            return s instanceof Select ? Txn.wrap((Select) s) : Txn.wrap((Mutation) s);
+        }).next(rs);
         IInvokableInstance node = state.cluster.get(rs.pickInt(state.topologyHistory.up()));
-        return new Property.SimpleCommand<>(node + ": " + stmt.kind() + "; epoch=" + state.currentEpoch.get(), s2 -> executeTxn(s2.cluster, node, cql, stmt.bindsEncoded()));
+        String msg = HISTORY_SHOWS_CQL ?
+                "\n" + stmt.visit(StandardVisitors.DEBUG).toCQL(new CQLFormatter.PrettyPrint()) + "\n"
+                : stmt.kind() == Statement.Kind.MUTATION ? ((Mutation) stmt).mutationKind().name() : stmt.kind().name();
+        return new Property.SimpleCommand<>(node + ":" + msg + "; epoch=" + state.currentEpoch.get(), s2 -> executeTxn(s2.cluster, node, stmt.toCQL(), stmt.bindsEncoded()));
     }
 
     private static boolean allowsMigration(TransactionalMode mode)
@@ -199,6 +229,12 @@ public class AccordTopologyMixupTest extends TopologyMixupTestBase<AccordTopolog
         {
             return metadata.keyspace;
         }
+
+        @Override
+        public String createSchema()
+        {
+            return metadata.toCqlString(false, false, false);
+        }
     }
 
     private static class AccordState extends State<Spec>

test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java

@@ -266,6 +266,12 @@ public class HarryTopologyMixupTest extends TopologyMixupTestBase<HarryTopologyM
         {
             return schema.keyspace;
         }
+
+        @Override
+        public String createSchema()
+        {
+            return schema.compile();
+        }
     }
 
     public class HarryState extends State<Spec>

test/distributed/org/apache/cassandra/fuzz/topology/TopologyMixupTestBase.java

@@ -469,6 +469,7 @@ public abstract class TopologyMixupTestBase<S extends TopologyMixupTestBase.Sche
     {
         String table();
         String keyspace();
+        String createSchema();
     }
 
     protected interface CommandGen<S extends Schema>
@@ -733,6 +734,8 @@ public abstract class TopologyMixupTestBase<S extends TopologyMixupTestBase.Sche
         {
             StringBuilder sb = new StringBuilder();
             sb.append("Yaml Config:\n").append(YamlConfigurationLoader.toYaml(this.yamlConfigOverrides));
+            String cql = schema.createSchema();
+            sb.append("\n-- Setup Schema\n").append(cql);
             sb.append("\nTopology:\n").append(topologyHistory);
             sb.append("\nCMS Voting Group: ").append(Arrays.toString(cmsGroup));
             if (epochHistory != null)

test/unit/org/apache/cassandra/cql3/CQLTester.java

@@ -536,7 +536,7 @@ public abstract class CQLTester
         ServerTestUtils.resetCMS();
         keyspaces.clear();
         tables.clear();
-        indexes.clear();;
+        indexes.clear();
         views.clear();
         types.clear();
         functions.clear();
@@ -2747,52 +2747,22 @@ public abstract class CQLTester
         });
     }
 
-//    protected String createIndexName()
-//    {
-//        String name = createSchemaElementName(SchemaElement.SchemaElementType.INDEX, null);
-//        indexes.add(name);
-//        return name;
-//    }
-//
-//    protected UntypedResultSet execute(org.apache.cassandra.cql3.ast.Statement stmt)
-//    {
-//        return executeFormattedQuery(stmt.toCQL(), stmt.bindsEncoded());
-//    }
-//
-//    protected ResultSet executeNet(ProtocolVersion protocolVersion, org.apache.cassandra.cql3.ast.Statement stmt)
-//    {
-//        return sessionNet(protocolVersion).execute(stmt.toCQL(), stmt.bindsEncoded());
-//    }
-//
-//    protected Mutation nonTransactionMutation(RandomSource rs, TableMetadata metadata)
-//    {
-//        return Generators.toGen(new ASTGenerators.MutationGenBuilder(metadata).withoutTransaction().build()).next(rs);
-//    }
+    protected String createIndexName()
+    {
+        String name = createSchemaElementName(SchemaElement.SchemaElementType.INDEX, null);
+        indexes.add(name);
+        return name;
+    }
 
-//    protected Select select(Mutation mutation)
-//    {
-//        // select * from table where <primaryKeys>
-//        return new Select(Collections.emptyList(),
-//                          Optional.of(new TableReference(mutation.table.keyspace, mutation.table.name)),
-//                          where(mutation.primaryKeys()),
-//                          Optional.empty(),
-//                          Optional.empty());
-//    }
+    protected UntypedResultSet execute(org.apache.cassandra.cql3.ast.Statement stmt)
+    {
+        return executeFormattedQuery(stmt.toCQL(), (Object[]) stmt.bindsEncoded());
+    }
 
-//    private Optional<Conditional> where(Map<Symbol, Expression> keys)
-//    {
-//        if (keys.isEmpty())
-//            throw new IllegalArgumentException("Unable to create a where clause from empty keys");
-//        Conditional.Builder builder = new Conditional.Builder();
-//        for (Map.Entry<Symbol, Expression> e : keys.entrySet())
-//            builder.where(Where.Inequalities.EQUAL, e.getKey(), e.getValue());
-//        return Optional.of(builder.build());
-//    }
-
-//    protected Object[][] rows(Mutation mutation)
-//    {
-//        return mutation.kind == Mutation.Kind.DELETE ? new Object[0][] : new Object[][]{row(mutation.toRowEncoded())};
-//    }
+    protected ResultSet executeNet(ProtocolVersion protocolVersion, org.apache.cassandra.cql3.ast.Statement stmt)
+    {
+        return sessionNet(protocolVersion).execute(stmt.toCQL(), (Object[]) stmt.bindsEncoded());
+    }
 
     @FunctionalInterface
     public interface CheckedFunction

test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java

@@ -23,12 +23,18 @@ import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Stream;
 
+import com.google.common.collect.ImmutableSet;
+
 import org.apache.cassandra.cql3.ast.Symbol.UnquotedSymbol;
 import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.AsciiType;
+import org.apache.cassandra.db.marshal.BooleanType;
 import org.apache.cassandra.db.marshal.CompositeType;
 import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UUIDType;
 import org.apache.cassandra.index.sai.StorageAttachedIndex;
 import org.apache.cassandra.index.sai.utils.IndexTermType;
 import org.apache.cassandra.schema.ColumnMetadata;
@@ -101,6 +107,10 @@ public class CreateIndexDDL implements Element
         }
     };
 
+    private static final Set<AbstractType<?>> SAI_EQ_ONLY = ImmutableSet.of(UTF8Type.instance, AsciiType.instance,
+                                                                            BooleanType.instance,
+                                                                            UUIDType.instance);
+
     public static final Indexer SAI = new Indexer()
     {
         @Override

test/unit/org/apache/cassandra/cql3/ast/Select.java

@@ -170,11 +170,11 @@ FROM [keyspace_name.] table_name
     public Stream<? extends Element> stream()
     {
         List<Element> es = new ArrayList<>(selections.size()
-                + (source.isPresent() ? 1 : 0)
-                + (where.isPresent() ? 1 : 0)
-                + (orderBy.isPresent() ? 1 : 0)
-                + (perPartitionLimit.isPresent() ? 1 : 0)
-                + (limit.isPresent() ? 1 : 0));
+                                           + (source.isPresent() ? 1 : 0)
+                                           + (where.isPresent() ? 1 : 0)
+                                           + (orderBy.isPresent() ? 1 : 0)
+                                           + (perPartitionLimit.isPresent() ? 1 : 0)
+                                           + (limit.isPresent() ? 1 : 0));
         es.addAll(selections);
         if (source.isPresent())
             es.add(source.get());
@@ -445,11 +445,11 @@ FROM [keyspace_name.] table_name
         public Select build()
         {
             return new Select((selections == null || selections.isEmpty()) ? Collections.emptyList() : ImmutableList.copyOf(selections),
-                    source,
-                    where.isEmpty() ? Optional.empty() : Optional.of(where.build()),
-                    orderBy.isEmpty() ? Optional.empty() : Optional.of(orderBy.build()),
-                    perPartitionLimit, limit,
-                    allowFiltering);
+                              source,
+                              where.isEmpty() ? Optional.empty() : Optional.of(where.build()),
+                              orderBy.isEmpty() ? Optional.empty() : Optional.of(orderBy.build()),
+                              perPartitionLimit, limit,
+                              allowFiltering);
         }
     }
 

test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java

@@ -18,14 +18,22 @@
 
 package org.apache.cassandra.cql3.statements;
 
-import org.apache.cassandra.cql3.ast.*;
+import java.util.Arrays;
+
+import org.assertj.core.api.Assertions;
 import org.junit.BeforeClass;
 import org.junit.Test;
 
 import org.apache.cassandra.SchemaLoader;
 import org.apache.cassandra.cql3.CQLStatement;
+import org.apache.cassandra.cql3.QueryOptions;
 import org.apache.cassandra.cql3.QueryProcessor;
 import org.apache.cassandra.cql3.ast.Conditional.Is;
+import org.apache.cassandra.cql3.ast.FunctionCall;
+import org.apache.cassandra.cql3.ast.Literal;
+import org.apache.cassandra.cql3.ast.Mutation;
+import org.apache.cassandra.cql3.ast.Select;
+import org.apache.cassandra.cql3.ast.Txn;
 import org.apache.cassandra.db.Keyspace;
 import org.apache.cassandra.exceptions.InvalidRequestException;
 import org.apache.cassandra.exceptions.SyntaxException;
@@ -36,7 +44,6 @@ import org.apache.cassandra.service.ClientState;
 import org.apache.cassandra.service.QueryState;
 import org.apache.cassandra.transport.Dispatcher;
 import org.apache.cassandra.transport.messages.ResultMessage;
-import org.assertj.core.api.Assertions;
 
 import static org.apache.cassandra.cql3.statements.TransactionStatement.DUPLICATE_TUPLE_NAME_MESSAGE;
 import static org.apache.cassandra.cql3.statements.TransactionStatement.EMPTY_TRANSACTION_MESSAGE;
@@ -396,9 +403,9 @@ public class TransactionStatementTest
 
             var txn = Txn.builder()
                          .addLet("a", Select.builder()
-                                 .table(tbl5())
-                                 .value("k", 1)
-                                 .build())
+                                            .table(tbl5())
+                                            .value("k", 1)
+                                            .build())
                          .addIf(new Is("a", Is.Kind.Null), mutation)
                          .build();
             Assertions.assertThatThrownBy(() -> prepare(txn.toCQL()))
@@ -476,10 +483,10 @@ public class TransactionStatementTest
         // this is blocked not because this isn't safe, but that the logic to handle this is currently in the read coordinator, which Accord doesn't call.
         // So rather than return bad results to users, IN w/ LIMIT is blocked... until we can fix
         Select select = Select.builder()
-                             .table(tbl(1))
-                             .in("k", 0, 1)
-                             .limit(Literal.of(1))
-                             .build();
+                              .table(tbl(1))
+                              .in("k", 0, 1)
+                              .limit(Literal.of(1))
+                              .build();
 
         Assertions.assertThatThrownBy(() -> prepare(Txn.wrap(select).toCQL()))
                   .isInstanceOf(InvalidRequestException.class)
@@ -493,6 +500,30 @@ public class TransactionStatementTest
                   .hasMessageContaining(String.format(NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", "at"));
     }
 
+    @Test
+    public void shouldRejectInClauseInLetWithBind()
+    {
+        Select select = Select.builder()
+                              .table(tbl(1))
+                              .in("k", 0, 1)
+                              .limit(1)
+                              .build();
+
+        TransactionStatement stmt = (TransactionStatement) prepare(Txn.wrap(select).toCQL());
+        QueryState state = QueryState.forInternalCalls();
+        Dispatcher.RequestTime now = Dispatcher.RequestTime.forImmediateExecution();
+        Assertions.assertThatThrownBy(() -> stmt.execute(state, QueryOptions.forInternalCalls(Arrays.asList(select.bindsEncoded())), now)).isInstanceOf(InvalidRequestException.class)
+                  .hasMessageContaining(String.format(NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", "at"));
+
+        Txn txn = Txn.builder()
+                   .addLet("a", select)
+                   .addReturnReferences("a.v")
+                   .build();
+        TransactionStatement stmt2 = (TransactionStatement) prepare(txn.toCQL());
+        Assertions.assertThatThrownBy(() -> stmt2.execute(state, QueryOptions.forInternalCalls(Arrays.asList(txn.bindsEncoded())), now)).isInstanceOf(InvalidRequestException.class)
+                  .hasMessageContaining(String.format(NO_PARTITION_IN_CLAUSE_WITH_LIMIT, "SELECT", "at"));
+    }
+
     @Test
     public void shouldRejectLetSelectOnNonTransactionalTable()
     {