CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 #3753
Conversation
| checkFalse(isSelectingMultipleClusterings(assignment.select, options), INCOMPLETE_PRIMARY_KEY_SELECT_MESSAGE, "LET assignment", assignment.select.source); | ||
| if (assignment.select.getRestrictions().keyIsInRelation()) |
There was a problem hiding this comment.
when LIMIT ? we are not able to validate, so need to rerun the validation here
| 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); |
There was a problem hiding this comment.
when LIMIT ? this would cause a NPE as options is null... we only don't NPE when the value is a literal...
| @@ -75,7 +75,7 @@ private static Query read(String ks, String table, List<ByteBuffer> keys) | |||
| assert !keys.isEmpty(); | |||
| Txn.Builder builder = new Txn.Builder(); | |||
| for (int i = 0; i < keys.size(); i++) | |||
| builder.addLet("row" + i, new Select.Builder().withWildcard().withTable(ks, table).withColumnEquals("pk", keys.get(i))); | |||
| builder.addLet("row" + i, new Select.Builder().withWildcard().withTable(ks, table).value("pk", keys.get(i))); | |||
There was a problem hiding this comment.
in order to make select/update/delete (places that support WHERE clause) consistent all WHERE based methods are now an interface and each builder implements... this is why this method got moved.
value might not be the best name, open to feedback to help readability... maybe equals?
| /** | ||
| * Should the history show the CQL? By default, this is off as its very verbose, but when debugging this can be helpful. | ||
| */ | ||
| private static final boolean HISTORY_SHOWS_CQL = false; |
There was a problem hiding this comment.
the changes to this class are not needed for this patch, nor for the model... but they are here for the following reasons
- this class is here to test topology changes, not be the fuzz test for accord... so by simplifying what we can do, the types, and the values... it makes debugging less crazy (I hate looking at blobs...)
- when you do hit a CQL issue (going to fix in trunk; delete with CAS against static fails) debugging can be annoying, so this flag lets you see the actual CQL run
| @@ -60,9 +59,9 @@ public static CompiledStatement select(Operations.SelectRow select, SchemaSpec s | |||
| for (int i = 0; i < schema.clusteringKeys.size(); i++) | |||
| { | |||
| ColumnSpec<?> column = schema.clusteringKeys.get(i); | |||
| builder.withWhere(Reference.of(new Symbol(column.name, column.type.asServerType())), | |||
There was a problem hiding this comment.
Reference as the default here was a dumb idea... I really wanted to make sure that txn references wouldn't break but this caused big usability issues... it also is really annoying to handle in the model as it adds far more states than Symbol does...
You can still use a Reference and this is tested with Txn... but removed the pattern for simplicity
| @@ -75,7 +75,7 @@ private static Query read(String ks, String table, List<ByteBuffer> keys) | |||
| assert !keys.isEmpty(); | |||
| Txn.Builder builder = new Txn.Builder(); | |||
| for (int i = 0; i < keys.size(); i++) | |||
| builder.addLet("row" + i, new Select.Builder().withWildcard().withTable(ks, table).withColumnEquals("pk", keys.get(i))); | |||
| builder.addLet("row" + i, new Select.Builder().wildcard().table(ks, table).value("pk", keys.get(i))); | |||
There was a problem hiding this comment.
in this case wildcard isn't needed, but it does protect the builder from something adding selections later on... because we not holding the reference after this line we really could remove, but left it for now.
| @@ -233,7 +233,7 @@ private static Where.Inequalities toInequalities(Relations.RelationKind kind) | |||
| private static CompiledStatement toCompiled(Select select) | |||
| { | |||
| // Select does not add ';' by default, but CompiledStatement expects this | |||
| String cql = select.toCQL().replace("\n", " ") + ";"; | |||
| String cql = select.toCQL(CQLFormatter.None.instance) + ';'; | |||
There was a problem hiding this comment.
for context Alex had a patch to add this behavior into Select and there was some back and forth about that... so he dropped the logic and used this replace... since this patch adds formatters we can do the same without needing the replace now.
| // select * from table where <primaryKeys> | ||
| return new Select(Collections.emptyList(), | ||
| Optional.of(new TableReference(Optional.of(mutation.table.keyspace), mutation.table.name)), | ||
| where(mutation.primaryKeys()), |
There was a problem hiding this comment.
supporting mutation.primaryKeys() is actually annoying as a mutation can impact multiple rows... This is also trying to hack together a bad model, so with the next patch adding a model it makes more sense to drop this logic in favor of the pending model.
| import static accord.utils.Property.qt; | ||
| import static java.lang.String.format; | ||
|
|
||
| public class RandomSchemaV2Test extends CQLTester |
There was a problem hiding this comment.
This test will be replaced by SingleNodeTableWalkerTest in the follow up patch. CQLTester queries the same as NODE_LOCAL, and some CQL operations act differently there than when coordinated... this issue impacts accord's BEGIN TRANSACTION which is why we keep disabling CQL features... I tried porting the model here and was bitten by this... so it really is better to avoid this pattern of test in CQLTester unless we do coordinated... I went down a rabbit hole of allowing coordinated execution, but that then overlaps with networking... it was honestly hard to define how this class adds value when SingleNodeTableWalkerTest lands as it has a larger search space, validation, etc.
| @Override | ||
| public Expression visit(Visitor v) | ||
| { | ||
| var u = v.visit(this); |
There was a problem hiding this comment.
the visit pattern is the same for each type, so documenting here as this is the first to show up in the diff
- the visitor could replace the full object, this is checked by calling the visitor directly
v.visit(this) - when working with sub elements we don't know if the element has sub-elements, so need to call
e.visit(v)so it can recursively do its work.
| @@ -47,7 +47,7 @@ public AbstractType<?> type() | |||
|
|
|||
| public ByteBuffer encode() | |||
| { | |||
| return ((AbstractType) type).decompose(value); | |||
| return value instanceof ByteBuffer ? (ByteBuffer) value : ((AbstractType) type).decompose(value); | |||
There was a problem hiding this comment.
the next patch adds a model, and to keep things simple all values are ByteBuffer right away, and not the java objects AbstractType expects, but that broke the ast package's Value types... so needed to explicitly handle in both types
| private static final Symbol pk = new Symbol("pk", Int32Type.instance); | ||
| private static final Symbol ck = new Symbol("ck", Int32Type.instance); |
There was a problem hiding this comment.
these are dead code... I just wanted to be complete...
| Mutation.InsertBuilder builder = Mutation.insert(TBL1); | ||
| builder.value("pk", 0); | ||
| builder.value("v1", 0); | ||
| builder.ttl(new Literal(42, Int32Type.instance)); |
There was a problem hiding this comment.
by default Bind is used when you use raw values. To make sure this test uses a Literal it needs to be explicit.
| return Collections.singletonList(this); | ||
| } | ||
|
|
||
| class Where implements Conditional |
There was a problem hiding this comment.
moved this class from its own file to here, so all impl are in one place... its a pattern that I started doing with this package so migrated to keep consistent... many elements are small so their own files seems wasteful
|
|
||
| class Where implements Conditional | ||
| { | ||
| public enum Inequality |
There was a problem hiding this comment.
renamed to be singular...
| sb.append(", "); | ||
| } | ||
| sb.setLength(sb.length() - 2); // ", " | ||
| sb.append(')'); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<? extends Element> stream() |
There was a problem hiding this comment.
bug found in testing... forgot to define the stream so the sub elements that contained bind would break... this method is used to find what bind values to send to the driver
| sb.append(" IS ").append(kind.cql); | ||
| } | ||
| } | ||
|
|
||
| class Builder | ||
| class And implements Conditional |
There was a problem hiding this comment.
moved from its own class to here to be consistent with the other types in this package
| interface EqBuilder<T extends EqBuilder<T>> | ||
| { | ||
| T value(Symbol symbol, Expression e); | ||
| default T value(String symbol, int e) | ||
| { | ||
| return value(new Symbol(symbol, Int32Type.instance), Bind.of(e)); | ||
| } | ||
| } | ||
|
|
||
| interface ConditionalBuilder<T extends ConditionalBuilder<T>> extends EqBuilder<T> |
There was a problem hiding this comment.
rather than have each builder copy/paste and let them get out of sync, made these interfaces to define what you can do with a Conditional, now all builders are consistent.
| @@ -55,6 +61,10 @@ default UnquotedSymbol longName() | |||
| return name(); | |||
| } | |||
| boolean supported(TableMetadata table, ColumnMetadata column); | |||
| default EnumSet<QueryType> supportedQueries(AbstractType<?> type) | |||
There was a problem hiding this comment.
this is needed by the model to know what operations could be done by the index. maintaining this is actually kind of a burden, so would be good to figure out how to ask the real impl....
| @Override | ||
| public EnumSet<QueryType> supportedQueries(AbstractType<?> type) | ||
| { | ||
| type = type.unwrap(); |
There was a problem hiding this comment.
Symbol.type is never reversed, you need Symbol.rawType() to get the reversed type... but since this works with types directly we have to be defensive off the bat...
|
|
||
| default void toCQL(StringBuilder sb) | ||
| { | ||
| toCQL(sb, 0); | ||
| toCQL(sb, CQLFormatter.None.instance); |
There was a problem hiding this comment.
before we would pretty print by default, but to avoid the extra allocation switched to None... I kinda feel anyone who cares should always define just to make sure this default can change over time.
| public static FunctionCall tokenByColumns(List<Symbol> columns) | ||
| { | ||
| return new FunctionCall("token", columns, BytesType.instance); | ||
| } | ||
|
|
||
| public static FunctionCall tokenByValue(List<? extends Value> values) | ||
| { | ||
| return new FunctionCall("token", values, BytesType.instance); |
There was a problem hiding this comment.
dead code in this patch, but used in the follow up model patch. This is here for the following
WHERE token(pk) = token(?)
| @@ -18,29 +18,35 @@ | |||
|
|
|||
| package org.apache.cassandra.cql3.ast; | |||
|
|
|||
| import java.util.stream.Stream; | |||
There was a problem hiding this comment.
this is a bad GH diff... AND was not renamed to StandardVisitors... they are unrelated, this is a 100% new file
| import org.apache.cassandra.schema.ColumnMetadata; | ||
|
|
||
| public class Symbol implements ReferenceExpression, Comparable<Symbol> | ||
| { | ||
| public final String symbol; | ||
| private final AbstractType<?> type; | ||
| public final boolean reversed; |
There was a problem hiding this comment.
having reverse type has only been a problem. Caleb and I talked about doing this for ColumnMetadata and did it here... has made life so much easier... there is very little code that cares about ordering, those code paths just call .rawType()....
|
|
||
| public String detailedName() | ||
| { | ||
| return symbol + " " + type.asCQL3Type() + (reversed ? " (reversed)" : ""); |
There was a problem hiding this comment.
this has been nice in the test in the next patch... below is a sample of the sai reverse bug
266: SELECT * FROM "arak0ffcoYDbQhwmjr1uXjr".tbl WHERE ck0 <= 0; -- ck0 varint (reversed), indexed with SAI
...
Caused by: java.lang.AssertionError: Missing rows:
pk0 | pk1 | ck0 | ck1 | v0
-3.793674428756257E-259 | '11:03:35.709809507' | -153195755 | 0x00000000000013009e00000000000000 | '2027-09-06T19:24:27.334Z'
Expected:
pk0 | pk1 | ck0 | ck1 | v0
-3.793674428756257E-259 | '11:03:35.709809507' | 0 | 0x0000000000001000bd00000000000000 | '2053-01-08T04:12:35.057Z'
-3.793674428756257E-259 | '11:03:35.709809507' | -153195755 | 0x00000000000013009e00000000000000 | '2027-09-06T19:24:27.334Z'
| case UPDATE: | ||
| mutation = Mutation.update(tbl5()) | ||
| .value(new Symbol("k", Int32Type.instance), Bind.of(1)) | ||
| .set(new Symbol("v", Int32Type.instance), Bind.of(2)) |
There was a problem hiding this comment.
this one line is different... and has to be different as UPDATE has both SET and WHERE blocks... you must be explicit.
| private final LinkedHashSet<Symbol> allColumns; | ||
| private final LinkedHashSet<Symbol> partitionColumns, clusteringColumns; | ||
| private final LinkedHashSet<Symbol> primaryColumns; | ||
| private final LinkedHashSet<Symbol> regularColumns, staticColumns, regularAndStaticColumns; |
There was a problem hiding this comment.
I hate showing the impl but I need a deterministic set and insertion order... in the follow up patch I am adding a new one, but to keep things simple this uses std java here
…ed to extend the harry model domain to handle more possible CQL states
dcapwell
force-pushed
the
CASSANDRA-20155
branch
from
ba3ac79 to
587676c
Compare
No description provided.