From 17dbba770bbb4efb5a0d8a72de48bc82767c8b52 Mon Sep 17 00:00:00 2001 From: David Capwell Date: Mon, 13 Jan 2025 10:11:03 -0800 Subject: [PATCH] Refactor the ast package to enable harry model based testing patch by David Capwell; reviewed by Abe Ratnofsky, Caleb Rackliffe for CASSANDRA-20155 --- .../cql3/statements/TransactionStatement.java | 11 +++- .../CoordinatorReadLatencyMetricTest.java | 4 +- .../topology/AccordTopologyMixupTest.java | 66 ++++++++++++++----- .../fuzz/topology/HarryTopologyMixupTest.java | 6 ++ .../fuzz/topology/TopologyMixupTestBase.java | 3 + .../org/apache/cassandra/cql3/CQLTester.java | 60 +++++------------ .../cassandra/cql3/ast/CreateIndexDDL.java | 10 +++ .../org/apache/cassandra/cql3/ast/Select.java | 20 +++--- .../statements/TransactionStatementTest.java | 49 +++++++++++--- 9 files changed, 147 insertions(+), 82 deletions(-) diff --git a/src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java b/src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java index c77b87ebc0..b95c1faa4a 100644 --- a/src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java +++ b/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 diff --git a/test/distributed/org/apache/cassandra/distributed/test/metrics/CoordinatorReadLatencyMetricTest.java b/test/distributed/org/apache/cassandra/distributed/test/metrics/CoordinatorReadLatencyMetricTest.java index 31de169030..6bbc09f6d1 100644 --- a/test/distributed/org/apache/cassandra/distributed/test/metrics/CoordinatorReadLatencyMetricTest.java +++ b/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(); diff --git a/test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java b/test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java index c51f4e714b..9e17c42896 100644 --- a/test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java +++ b/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 { 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 TRANSACTIONAL_MODES = Stream.of(TransactionalMode.values()).filter(t -> t.accordIsEnabled).collect(Collectors.toList()); @@ -91,21 +113,27 @@ public class AccordTopologyMixupTest extends TopologyMixupTestBase, Void, ?> cqlOperation(RandomSource rs, State state, Gen 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 diff --git a/test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java b/test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java index b969eff6df..816c38f11a 100644 --- a/test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java +++ b/test/distributed/org/apache/cassandra/fuzz/topology/HarryTopologyMixupTest.java @@ -266,6 +266,12 @@ public class HarryTopologyMixupTest extends TopologyMixupTestBase diff --git a/test/distributed/org/apache/cassandra/fuzz/topology/TopologyMixupTestBase.java b/test/distributed/org/apache/cassandra/fuzz/topology/TopologyMixupTestBase.java index d3ab7fea08..9bc25afd85 100644 --- a/test/distributed/org/apache/cassandra/fuzz/topology/TopologyMixupTestBase.java +++ b/test/distributed/org/apache/cassandra/fuzz/topology/TopologyMixupTestBase.java @@ -469,6 +469,7 @@ public abstract class TopologyMixupTestBase @@ -733,6 +734,8 @@ public abstract class TopologyMixupTestBase -// 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 where(Map 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 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 diff --git a/test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java b/test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java index ce86d6bbb5..3cdf2eba0c 100644 --- a/test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java +++ b/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> SAI_EQ_ONLY = ImmutableSet.of(UTF8Type.instance, AsciiType.instance, + BooleanType.instance, + UUIDType.instance); + public static final Indexer SAI = new Indexer() { @Override diff --git a/test/unit/org/apache/cassandra/cql3/ast/Select.java b/test/unit/org/apache/cassandra/cql3/ast/Select.java index eea9ded9ad..10d98dee63 100644 --- a/test/unit/org/apache/cassandra/cql3/ast/Select.java +++ b/test/unit/org/apache/cassandra/cql3/ast/Select.java @@ -170,11 +170,11 @@ FROM [keyspace_name.] table_name public Stream stream() { List 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); } } diff --git a/test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java b/test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java index 2e3371e542..dd4c816e84 100644 --- a/test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java +++ b/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() {