You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2024/12/19 18:16:08 UTC
[PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
dcapwell opened a new pull request, #3753:
URL: https://github.com/apache/cassandra/pull/3753
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893014036
##########
test/unit/org/apache/cassandra/cql3/ast/StandardVisitors.java:
##########
@@ -18,29 +18,35 @@
package org.apache.cassandra.cql3.ast;
-import java.util.stream.Stream;
Review Comment:
this is a bad GH diff... `AND` was not renamed to `StandardVisitors`... they are unrelated, this is a 100% new file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893020295
##########
test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java:
##########
@@ -371,23 +376,38 @@ public void shouldRejectLetSelectWithIncompletePartitionKey()
@Test
public void shouldRejectTTL()
{
- Mutation.Builder builder = Mutation.builder(tbl5())
- .value("k", 1)
- .value("v", 2)
- .ttl(42);
for (Mutation.Kind kind : Mutation.Kind.values())
{
if (kind == Mutation.Kind.DELETE) continue; // deletes don't support TTL
- Mutation mutation = builder.kind(kind).build();
+ Mutation mutation;
+ switch (kind)
+ {
+ case INSERT:
+ mutation = Mutation.insert(tbl5())
+ .value(new Symbol("k", Int32Type.instance), Bind.of(1))
+ .value(new Symbol("v", Int32Type.instance), Bind.of(2))
+ .ttl(42)
+ .build();
+ break;
+ case UPDATE:
+ mutation = Mutation.update(tbl5())
+ .value(new Symbol("k", Int32Type.instance), Bind.of(1))
+ .set(new Symbol("v", Int32Type.instance), Bind.of(2))
Review Comment:
this one line is different... and has to be different as `UPDATE` has both `SET` and `WHERE` blocks... you must be explicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1901980343
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
Review Comment:
I think it's a known error, but test is failing for me locally with:
```
accord.utils.Property$PropertyError: Property error detected:
Seed = 3447854785953799011
Examples = 1
Pure = true
Error: org.apache.cassandra.exceptions.InvalidRequestException: DELETE statements must restrict all PRIMARY KEY columns with equality relations in order to delete non static columns
Steps: 20
Values:
State: Yaml Config:
---
commitlog_sync: group
commitlog_sync_group_window: 7ms
commitlog_disk_access_mode: legacy
memtable_allocation_type: unslabbed_heap_buffers
memtable:
configurations:
TrieMemtable:
class_name: TrieMemtable
ShardedSkipListMemtable:
class_name: ShardedSkipListMemtable
parameters:
serialize_writes: true
default:
inherits: ShardedSkipListMemtable
SkipListMemtable:
class_name: SkipListMemtable
sstable:
selected_format: bti
disk_access_mode: mmap_index_only
-- Setup Schema
CREATE TABLE distributed_test_keyspace.tbl (
pk0 boolean,
ck0 text,
v0 uuid,
v1 float,
v2 tinyint,
v3 ascii,
v4 'org.apache.cassandra.db.marshal.LexicalUUIDType',
PRIMARY KEY (pk0, ck0)
) WITH CLUSTERING ORDER BY (ck0 ASC)
AND additional_write_policy = '99p'
AND allow_auto_snapshot = true
AND bloom_filter_fp_chance = 0.01
AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'}
AND cdc = false
AND comment = ''
AND compaction = {'class': 'org.apache.cassandra.db.compaction.SizeTieredCompactionStrategy', 'max_threshold': '32', 'min_threshold': '4'}
AND compression = {'chunk_length_in_kb': '16', 'class': 'org.apache.cassandra.io.compress.LZ4Compressor'}
AND memtable = 'default'
AND crc_check_chance = 1.0
AND fast_path = 'keyspace'
AND default_time_to_live = 0
AND extensions = {}
AND gc_grace_seconds = 864000
AND incremental_backups = true
AND max_index_interval = 2048
AND memtable_flush_period_in_ms = 0
AND min_index_interval = 128
AND read_repair = 'BLOCKING'
AND transactional_mode = 'off'
AND transactional_migration_from = 'none'
AND speculative_retry = '99p';
Topology:
Node1: status=Up, tokens=[1353573382]
Node2: status=Up, tokens=[456996056]
CMS Voting Group: [1, 2]
Epochs:
epoch | kind | transformation
16 | BEGIN_CONSENSUS_MIGRATION_FOR_TABLE_AND_RANGE | org.apache.cassandra.tcm.transformations.BeginConsensusMigrationForTableAndRange@68b70cad
15 | SCHEMA_CHANGE | AlterSchema{schemaTransformation=AlterOptions (distributed_test_keyspace, tbl)}
14 | SCHEMA_CHANGE | AlterSchema{schemaTransformation=CreateTableStatement (distributed_test_keyspace, tbl)}
13 | ADVANCE_CMS_RECONFIGURATION | AdvanceCMSReconfiguration{idx=2, current=FinishReconfiguration(), diff=Diff{additions=[], removals=[...
12 | ADVANCE_CMS_RECONFIGURATION | AdvanceCMSReconfiguration{idx=1, current=FinishCMSReconfiguration(), diff=Diff{additions=[], removal...
11 | ADVANCE_CMS_RECONFIGURATION | AdvanceCMSReconfiguration{idx=0, current=StartAddToCMS(NodeId{id=2}), diff=Diff{additions=[NodeId{id...
10 | PREPARE_COMPLEX_CMS_RECONFIGURATION | PrepareCMSReconfiguration#Complex{replicationParams=ReplicationParams{class=org.apache.cassandra.loc...
9 | SCHEMA_CHANGE | AlterSchema{schemaTransformation=CreateKeyspaceStatement (distributed_test_keyspace)}
8 | SCHEMA_CHANGE | AlterSchema{schemaTransformation=AlterKeyspaceStatement (system_traces)}
7 | SCHEMA_CHANGE | AlterSchema{schemaTransformation=AlterKeyspaceStatement (system_auth)}
6 | UNSAFE_JOIN | UnsafeJoin{nodeId=NodeId{id=2}, tokens=[456996056], joinTokenRing=true, streamData=false}
5 | REGISTER | Register{addresses=NodeAddresses{broadcastAddress=/127.0.0.2:7012, localAddress=/127.0.0.2:7012, nat...
4 | UNSAFE_JOIN | UnsafeJoin{nodeId=NodeId{id=1}, tokens=[1353573382], joinTokenRing=true, streamData=false}
3 | REGISTER | Register{addresses=NodeAddresses{broadcastAddress=/127.0.0.1:7012, localAddress=/127.0.0.1:7012, nat...
2 | INITIALIZE_CMS | Initialize{baseState = Epoch{epoch=1}}
1 | PRE_INITIALIZE_CMS | PreInitialize{addr=null}
: org.apache.cassandra.fuzz.topology.AccordTopologyMixupTest.AccordState
History:
1: node1:TXN; epoch=16; Duration PT0.305886666S
2: node1:TXN; epoch=16; Duration PT0.046933292S
3: node1:TXN; epoch=16; Duration PT0.024392083S
4: node2:DELETE; epoch=16; Duration PT0.003137125S
```
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -109,4 +109,14 @@ public AbstractType<?> type()
{
return right.type();
}
+
+ @Override
+ public Expression visit(Visitor v)
+ {
+ var u = v.visit(this);
Review Comment:
Left a comment about this on Visitor - could you add this as a comment on that interface?
##########
test/unit/org/apache/cassandra/cql3/ast/Visitor.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import java.util.List;
+
+public interface Visitor
Review Comment:
Could you add a top-level comment explaining this interface?
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
@@ -59,23 +66,38 @@
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 final boolean HISTORY_SHOWS_CQL = false;
Review Comment:
Would be cool to have this set when a failure is retried, might be worth including some failure retry usability improvements later on. Could also make non-final so it can be set in `AccordTopologyMixupTest#preCheck`, since that method will presumably be used to retry failures with a given seed.
##########
test/harry/main/org/apache/cassandra/harry/cql/SelectHelper.java:
##########
@@ -60,9 +60,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())),
- toInequalities(Relations.RelationKind.EQ),
- new Bind(ck[i], column.type.asServerType()));
+ builder.where(new Symbol(column.name, column.type.asServerType()),
+ toInequalities(Relations.RelationKind.EQ),
Review Comment:
Nit: you renamed to Inequality (singular) elsewhere
##########
test/unit/org/apache/cassandra/utils/ASTGenerators.java:
##########
@@ -76,7 +76,7 @@
public class ASTGenerators
{
- static Gen<Value> valueGen(Object value, AbstractType<?> type)
+ static <T> Gen<Value> valueGen(Object value, AbstractType<?> type)
Review Comment:
Unused type param intentional?
##########
test/unit/org/apache/cassandra/cql3/ast/Select.java:
##########
@@ -270,124 +301,107 @@ public OrderBy build()
}
}
- public static class Builder
+ public static class Builder implements Conditional.ConditionalBuilder<Builder>
{
- private boolean filtering = false;
@Nullable // null means wildcard
private List<Expression> selections = new ArrayList<>();
private Optional<TableReference> source = Optional.empty();
private Conditional.Builder where = new Conditional.Builder();
private OrderBy.Builder orderBy = new OrderBy.Builder();
private Optional<Value> limit = Optional.empty();
+ private boolean allowFiltering = false;
- public Builder withWildcard()
+ public Builder wildcard()
{
if (selections != null && !selections.isEmpty())
throw new IllegalStateException("Attempted to use * for selection but existing selections exist: " + selections);
Review Comment:
TIL we don't support column references and wildcards in the same CQL query (unlike Postgres, for example):
```
> SELECT key, bootstrapped, * FROM system.local;
SyntaxException: line 1:26 no viable alternative at input '*' (SELECT key, bootstrapped, [*]...)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3753: CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155
URL: https://github.com/apache/cassandra/pull/3753
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893001403
##########
test/unit/org/apache/cassandra/cql3/ast/Conditional.java:
##########
@@ -34,32 +41,171 @@ default AbstractType<?> type()
return BooleanType.instance;
}
+ @Override
+ default Conditional visit(Visitor v)
+ {
+ return v.visit(this);
+ }
+
+ default List<Conditional> simplify()
+ {
+ return Collections.singletonList(this);
+ }
+
+ class Where implements Conditional
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892978165
##########
test/unit/org/apache/cassandra/cql3/RandomSchemaV2Test.java:
##########
@@ -1,304 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you 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.cql3;
-
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.Set;
-import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
-
-import org.junit.BeforeClass;
-import org.junit.Test;
-
-import accord.utils.Gen;
-import accord.utils.Gens;
-import accord.utils.RandomSource;
-import org.apache.cassandra.config.DatabaseDescriptor;
-import org.apache.cassandra.cql3.ast.CreateIndexDDL;
-import org.apache.cassandra.cql3.ast.CreateIndexDDL.Indexer;
-import org.apache.cassandra.cql3.ast.Mutation;
-import org.apache.cassandra.cql3.ast.ReferenceExpression;
-import org.apache.cassandra.cql3.ast.Select;
-import org.apache.cassandra.cql3.ast.Symbol;
-import org.apache.cassandra.cql3.ast.TableReference;
-import org.apache.cassandra.cql3.ast.Txn;
-import org.apache.cassandra.db.Keyspace;
-import org.apache.cassandra.db.marshal.AbstractType;
-import org.apache.cassandra.db.marshal.CompositeType;
-import org.apache.cassandra.db.marshal.MapType;
-import org.apache.cassandra.db.marshal.UTF8Type;
-import org.apache.cassandra.index.Index;
-import org.apache.cassandra.index.SecondaryIndexManager;
-import org.apache.cassandra.schema.ColumnMetadata;
-import org.apache.cassandra.schema.KeyspaceMetadata;
-import org.apache.cassandra.schema.Schema;
-import org.apache.cassandra.schema.TableMetadata;
-import org.apache.cassandra.service.consensus.TransactionalMode;
-import org.apache.cassandra.service.consensus.migration.TransactionalMigrationFromMode;
-import org.apache.cassandra.transport.ProtocolVersion;
-import org.apache.cassandra.utils.AbstractTypeGenerators;
-import org.apache.cassandra.utils.CassandraGenerators;
-import org.awaitility.Awaitility;
-
-import static accord.utils.Property.qt;
-import static java.lang.String.format;
-
-public class RandomSchemaV2Test extends CQLTester
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909341820
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatter.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import static org.apache.cassandra.cql3.ast.Elements.newLine;
+
+public interface CQLFormatter
+{
+ void group(StringBuilder sb);
+ void endgroup(StringBuilder sb);
+
+ void subgroup(StringBuilder sb);
+ void endsubgroup(StringBuilder sb);
+
+ void section(StringBuilder sb);
+ void element(StringBuilder sb);
+
+ class PrettyPrint implements CQLFormatter
+ {
+ private final int spacePerGroup = 2;
Review Comment:
```suggestion
private static final int spacePerGroup = 2;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909898837
##########
test/unit/org/apache/cassandra/cql3/ast/Symbol.java:
##########
@@ -92,12 +95,35 @@ public AbstractType<?> type()
return type;
}
+ public AbstractType<?> rawType()
+ {
+ if (reversed) return ReversedType.getInstance(type);
+ return type;
+ }
+
@Override
public String name()
{
return symbol;
}
+ /**
+ * Same as {@link #visit(Visitor)} but has the restriction that the return type may not be changed away from {@link Symbol}.
+ */
+ public Symbol visit(String type, Visitor v)
Review Comment:
nit: Could use the `Mutation.Kind` enum here instead of a string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1911481292
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatter.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import static org.apache.cassandra.cql3.ast.Elements.newLine;
+
+public interface CQLFormatter
Review Comment:
ill need to think about this, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1911488516
##########
test/unit/org/apache/cassandra/cql3/ast/Symbol.java:
##########
@@ -92,12 +95,35 @@ public AbstractType<?> type()
return type;
}
+ public AbstractType<?> rawType()
+ {
+ if (reversed) return ReversedType.getInstance(type);
+ return type;
+ }
+
@Override
public String name()
{
return symbol;
}
+ /**
+ * Same as {@link #visit(Visitor)} but has the restriction that the return type may not be changed away from {@link Symbol}.
+ */
+ public Symbol visit(String type, Visitor v)
Review Comment:
currently I could but that then couples `Symbol` with `Mutation`. The `String type` is just for better assert logic so rather not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1907936159
##########
test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java:
##########
@@ -483,6 +500,30 @@ public void shouldRejectInClauseInLet()
.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)
+ .addReturn(select)
+ .build();
+ TransactionStatement stmt2 = (TransactionStatement) prepare(txn
+ .toCQL());
Review Comment:
nit: wacky newline
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902203456
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
Review Comment:
fixed now. The seed now fails due to host replacement issues...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892981232
##########
test/unit/org/apache/cassandra/cql3/ast/Bind.java:
##########
@@ -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);
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892961330
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
@@ -59,23 +66,38 @@
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 final boolean HISTORY_SHOWS_CQL = false;
Review Comment:
the changes to this class are not needed for this patch, nor for the model... but they are here for the following reasons
1) 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...)
2) 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909351414
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatter.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import static org.apache.cassandra.cql3.ast.Elements.newLine;
+
+public interface CQLFormatter
Review Comment:
Is there anything useful we can get out of `CqlBuilder` (i.e. if we make its indent size configurable) rather than having both classes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1907938356
##########
test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java:
##########
@@ -483,6 +500,30 @@ public void shouldRejectInClauseInLet()
.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)
+ .addReturn(select)
Review Comment:
nit: Would it make sense to return something like `SELECT a.whatever` here instead of a standard SELECT? I guess that would ensure we're hitting the error because of the LET specifically?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909342533
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatterPrettyPrintTest.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import org.junit.Test;
+
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.schema.TableMetadata;
+import org.assertj.core.api.Assertions;
+
+public class CQLFormatterPrettyPrintTest
+{
+ private static TableMetadata TBL1 = TableMetadata.builder("ks", "tbl")
Review Comment:
```suggestion
private static final TableMetadata TBL1 = TableMetadata.builder("ks", "tbl")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893003642
##########
test/unit/org/apache/cassandra/cql3/ast/Conditional.java:
##########
@@ -77,29 +223,158 @@ public enum Kind
}
}
+ public final ReferenceExpression reference;
public final Kind kind;
- public final Reference reference;
public Is(String symbol, Kind kind)
{
- this(Reference.of(Symbol.unknownType(symbol)), kind);
+ this(Symbol.unknownType(symbol), kind);
}
- public Is(Reference reference, Kind kind)
+ public Is(ReferenceExpression reference, Kind kind)
{
- this.kind = kind;
this.reference = reference;
+ this.kind = kind;
}
@Override
- public void toCQL(StringBuilder sb, int indent)
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
{
- reference.toCQL(sb, indent);
+ reference.toCQL(sb, formatter);
sb.append(" IS ").append(kind.cql);
}
}
- class Builder
+ class And implements Conditional
+ {
+ public final Conditional left, right;
+
+ public And(Conditional left, Conditional right)
+ {
+ this.left = left;
+ this.right = right;
+ }
+
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
+ {
+ left.toCQL(sb, formatter);
+ sb.append(" AND ");
+ right.toCQL(sb, formatter);
+ }
+
+ @Override
+ public Stream<? extends Element> stream()
+ {
+ return Stream.of(left, right);
+ }
+
+ @Override
+ public Conditional visit(Visitor v)
+ {
+ var u = v.visit(this);
+ if (u != this) return u;
+ Conditional left = this.left.visit(v);
+ Conditional right = this.right.visit(v);
+ if (this.left == left && this.right == right) return this;
+ return new And(left, right);
+ }
+
+ @Override
+ public List<Conditional> simplify()
+ {
+ List<Conditional> result = new ArrayList<>();
+ result.addAll(left.simplify());
+ result.addAll(right.simplify());
+ return result;
+ }
+ }
+
+ 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>
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893005999
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -134,6 +148,15 @@ else if (((isFrozen(type) && !type.isVector()) || StorageAttachedIndex.SUPPORTED
return true;
return false;
}
+
+ @Override
+ public EnumSet<QueryType> supportedQueries(AbstractType<?> type)
+ {
+ type = type.unwrap();
Review 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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1911467175
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -55,6 +61,10 @@ default UnquotedSymbol longName()
return name();
}
boolean supported(TableMetadata table, ColumnMetadata column);
+ default EnumSet<QueryType> supportedQueries(AbstractType<?> type)
Review Comment:
thanks, ill think about that... right now this is at the algo level and not the actual index level, so I would need to define a primitive that closer maps to `Index` (which I kinda have in my tests just for toString)...
Right now we really only support SAI as everything else is not really maintained. I can kinda hack and use `org.apache.cassandra.index.sai.utils.IndexTermType#supports` rather than my current logic too...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892979805
##########
test/unit/org/apache/cassandra/cql3/ast/AssignmentOperator.java:
##########
@@ -109,4 +109,14 @@ public AbstractType<?> type()
{
return right.type();
}
+
+ @Override
+ public Expression visit(Visitor v)
+ {
+ var u = v.visit(this);
Review Comment:
the `visit` pattern is the same for each type, so documenting here as this is the first to show up in the diff
1) the visitor could replace the full object, this is checked by calling the visitor directly `v.visit(this)`
2) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902052427
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
@@ -59,23 +66,38 @@
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 final boolean HISTORY_SHOWS_CQL = false;
Review Comment:
from a code point of view what does that look like? if the test defines the seed then assume its a failure retry?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902053668
##########
test/unit/org/apache/cassandra/cql3/ast/Select.java:
##########
@@ -270,124 +301,107 @@ public OrderBy build()
}
}
- public static class Builder
+ public static class Builder implements Conditional.ConditionalBuilder<Builder>
{
- private boolean filtering = false;
@Nullable // null means wildcard
private List<Expression> selections = new ArrayList<>();
private Optional<TableReference> source = Optional.empty();
private Conditional.Builder where = new Conditional.Builder();
private OrderBy.Builder orderBy = new OrderBy.Builder();
private Optional<Value> limit = Optional.empty();
+ private boolean allowFiltering = false;
- public Builder withWildcard()
+ public Builder wildcard()
{
if (selections != null && !selections.isEmpty())
throw new IllegalStateException("Attempted to use * for selection but existing selections exist: " + selections);
Review Comment:
tested in sqlite
```
sqlite> create table foo(pk smallint);
sqlite> select pk, * from foo;
sqlite>
```
would be nice to support =D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892957931
##########
src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java:
##########
@@ -492,8 +500,9 @@ private static void validate(SelectStatement prepared)
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);
Review Comment:
when `LIMIT ?` this would cause a NPE as `options` is `null`... we only don't NPE when the value is a literal...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892957359
##########
src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java:
##########
@@ -387,7 +387,15 @@ public ResultMessage execute(QueryState state, QueryOptions options, Dispatcher.
// 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())
Review Comment:
when `LIMIT ?` we are not able to validate, so need to rerun the validation here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893014036
##########
test/unit/org/apache/cassandra/cql3/ast/StandardVisitors.java:
##########
@@ -18,29 +18,35 @@
package org.apache.cassandra.cql3.ast;
-import java.util.stream.Stream;
Review Comment:
this is a bad GH diff... `AND` was not renamed to `StandardVisitors`...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909296787
##########
test/unit/org/apache/cassandra/cql3/ast/Visitor.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Stream;
+
+import accord.utils.Invariants;
+
+/**
+ * Visits and conditionally replaces an element in the ast. Each "visit" method offers the ability to be notified when the
+ * desired type is found in the ast tree, and allows the visitor to replace the current element (return the element unchanged
+ * if no mutations are desired, return a different element if a change is desired).
+ *
+ * For elements that support visitors, the following pattern should be respected.
+ * <ol>
+ * <li>check the top level element first
+ * {code}
+ * var u = visitor.visit(this);
+ * if (u != this) return u;
+ * {code}</li>
+ * <li>check each sub element; replacing the top level element if any sub-elements are changed
+ * {code}
+ * boolean updated = false;
+ * var a = a.visit(visitor);
+ * updated |= (a != this.a);
+ * ...
+ * return !updated ? this : copy...
+ * {code}
+ * </li>
+ * </ol>
+ */
+public interface Visitor
+{
+ default Statement visit(Statement s)
+ {
+ return s;
+ }
+
+ default Expression visit(Expression e)
+ {
+ if (e instanceof Value) return visit((Value) e);
+ if (e instanceof ReferenceExpression) return visit((ReferenceExpression) e);
+ return e;
+ }
+
+ default Conditional visit(Conditional c) { return c; }
+
+ default ReferenceExpression visit(ReferenceExpression r)
+ {
+ return r;
+ }
+
+ default Value visit(Value v) { return v; }
+
+ class CompositeVisitor implements Visitor
+ {
+ private final List<Visitor> visitors;
+
+ private CompositeVisitor(List<Visitor> visitors)
+ {
+ this.visitors = visitors;
+ }
+
+ public static CompositeVisitor of(Visitor... visitors)
+ {
+ return of(Arrays.asList(visitors));
+ }
+
+ public static CompositeVisitor of(List<Visitor> visitors)
+ {
+ Invariants.checkArgument(!visitors.isEmpty(), "Visitors may not be empty");
+
+ if (!Stream.of(visitors).anyMatch(v -> v instanceof CompositeVisitor))
Review Comment:
```suggestion
if (Stream.of(visitors).noneMatch(v -> v instanceof CompositeVisitor))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902208001
##########
test/unit/org/apache/cassandra/cql3/ast/Visitor.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import java.util.List;
+
+public interface Visitor
Review Comment:
pushed docs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893016615
##########
test/unit/org/apache/cassandra/cql3/ast/Symbol.java:
##########
@@ -26,12 +26,14 @@
import org.apache.cassandra.cql3.ReservedKeywords;
import org.apache.cassandra.db.marshal.AbstractType;
import org.apache.cassandra.db.marshal.BytesType;
+import org.apache.cassandra.db.marshal.ReversedType;
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;
Review 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()`....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909358037
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -55,6 +61,10 @@ default UnquotedSymbol longName()
return name();
}
boolean supported(TableMetadata table, ColumnMetadata column);
+ default EnumSet<QueryType> supportedQueries(AbstractType<?> type)
Review Comment:
`Index#supportsExpression()` does this if you can give it a column and operator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909895941
##########
test/unit/org/apache/cassandra/cql3/ast/StandardVisitors.java:
##########
@@ -18,29 +18,35 @@
package org.apache.cassandra.cql3.ast;
-import java.util.stream.Stream;
+import java.util.Arrays;
Review Comment:
`Arrays` is unused?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892959103
##########
test/distributed/org/apache/cassandra/distributed/test/accord/NewSchemaTest.java:
##########
@@ -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)));
Review 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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892974485
##########
test/unit/org/apache/cassandra/cql3/CQLTester.java:
##########
@@ -2669,36 +2661,6 @@ protected ResultSet executeNet(ProtocolVersion protocolVersion, org.apache.cassa
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 Select select(Mutation mutation)
- {
- // 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()),
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892968887
##########
test/distributed/org/apache/cassandra/distributed/test/accord/NewSchemaTest.java:
##########
@@ -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)));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892973035
##########
test/harry/main/org/apache/cassandra/harry/cql/SelectHelper.java:
##########
@@ -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) + ';';
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892984433
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatterPrettyPrintTest.java:
##########
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import org.junit.Test;
+
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.schema.TableMetadata;
+import org.assertj.core.api.Assertions;
+
+public class CQLFormatterPrettyPrintTest
+{
+ private static TableMetadata TBL1 = TableMetadata.builder("ks", "tbl")
+ .addPartitionKeyColumn("pk", Int32Type.instance)
+ .addClusteringColumn("ck", Int32Type.instance)
+ .addRegularColumn("v0", Int32Type.instance)
+ .addRegularColumn("v1", Int32Type.instance)
+ .build();
+ private static final Symbol pk = new Symbol("pk", Int32Type.instance);
+ private static final Symbol ck = new Symbol("ck", Int32Type.instance);
+ private static final Symbol v0 = new Symbol("v0", Int32Type.instance);
+ private static final Symbol v1 = new Symbol("v1", Int32Type.instance);
+
+ @Test
+ public void selectTest()
+ {
+ Select select = select();
+ Assertions.assertThat(select.toCQL(format())).isEqualTo("SELECT *\n" +
+ "FROM ks.tbl\n" +
+ "WHERE foo = ?\n" +
+ "LIMIT ?");
+ }
+
+ @Test
+ public void insertTest()
+ {
+ Mutation.InsertBuilder builder = Mutation.insert(TBL1);
+ builder.value("pk", 0);
+ builder.value("v1", 0);
+ builder.ttl(new Literal(42, Int32Type.instance));
Review Comment:
by default `Bind` is used when you use raw values. To make sure this test uses a `Literal` it needs to be explicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892964144
##########
test/harry/main/org/apache/cassandra/harry/cql/SelectHelper.java:
##########
@@ -146,7 +145,7 @@ public static CompiledStatement select(Operations.SelectCustom select, SchemaSpe
}
}
- builder.allowFiltering();
+ builder.withAllowfiltering();
Review Comment:
`allowFiltering` was the only method in the class that wasn't prefixed with `with`, so migrated to match the style of the class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893018010
##########
test/unit/org/apache/cassandra/cql3/ast/Symbol.java:
##########
@@ -92,12 +95,35 @@ public AbstractType<?> type()
return type;
}
+ public AbstractType<?> rawType()
+ {
+ if (reversed) return ReversedType.getInstance(type);
+ return type;
+ }
+
@Override
public String name()
{
return symbol;
}
+ /**
+ * Same as {@link #visit(Visitor)} but has the restriction that the return type may not be changed away from {@link Symbol}.
+ */
+ public Symbol visit(String type, Visitor v)
+ {
+ var u = v.visit(this);
+ if (u == this) return this;
+ if (!(u instanceof Symbol))
+ throw new IllegalStateException("Visitor converted a Symbol to " + u.getClass().getSimpleName() + "; this is not supported with " + type);
+ return (Symbol) u;
+ }
+
+ public String detailedName()
+ {
+ return symbol + " " + type.asCQL3Type() + (reversed ? " (reversed)" : "");
Review 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'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893025321
##########
test/unit/org/apache/cassandra/utils/ASTGenerators.java:
##########
@@ -254,40 +321,54 @@ private static Gen<Map<Symbol, Expression>> partitionKeyGen(TableMetadata metada
public static class MutationGenBuilder
{
+ public enum DeleteKind { Partition, Row, Column }
private final TableMetadata metadata;
- private final Set<Symbol> allColumns;
- private final Set<Symbol> primaryColumns;
- private final Set<Symbol> nonPrimaryColumns;
+ private final LinkedHashSet<Symbol> allColumns;
+ private final LinkedHashSet<Symbol> partitionColumns, clusteringColumns;
+ private final LinkedHashSet<Symbol> primaryColumns;
+ private final LinkedHashSet<Symbol> regularColumns, staticColumns, regularAndStaticColumns;
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893004640
##########
test/unit/org/apache/cassandra/cql3/ast/CreateIndexDDL.java:
##########
@@ -55,6 +61,10 @@ default UnquotedSymbol longName()
return name();
}
boolean supported(TableMetadata table, ColumnMetadata column);
+ default EnumSet<QueryType> supportedQueries(AbstractType<?> type)
Review 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....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893001739
##########
test/unit/org/apache/cassandra/cql3/ast/Conditional.java:
##########
@@ -34,32 +41,171 @@ default AbstractType<?> type()
return BooleanType.instance;
}
+ @Override
+ default Conditional visit(Visitor v)
+ {
+ return v.visit(this);
+ }
+
+ default List<Conditional> simplify()
+ {
+ return Collections.singletonList(this);
+ }
+
+ class Where implements Conditional
+ {
+ public enum Inequality
Review Comment:
renamed to be singular...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902051529
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
Review Comment:
I think that is https://issues.apache.org/jira/browse/CASSANDRA-20163, once I wrap up that patch ill confirm that this seed hit that case...
TL-DR; delete whole partition if static column matches a condition is blocked when it shouldn't be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902184081
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
Review Comment:
the delete is
```
4: node2:
DELETE
FROM distributed_test_keyspace.tbl
WHERE
pk0 = true
IF EXISTS
; epoch=16; Duration PT0.00608525S
```
so, it relates to https://issues.apache.org/jira/browse/CASSANDRA-20163 as it tried to support this, but at this moment this is a costly query so that patch doesn't add support... To improve stability this example shouldn't be generated... ill fix the generator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1902204350
##########
test/distributed/org/apache/cassandra/fuzz/topology/AccordTopologyMixupTest.java:
##########
@@ -59,23 +66,38 @@
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 final boolean HISTORY_SHOWS_CQL = false;
Review Comment:
in the example of how to do the seed I added
```
// HISTORY_SHOWS_CQL = true; // uncomment if the CQL done should be included in the history
```
its not automatic but tells the person it exists at least?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1907938356
##########
test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java:
##########
@@ -483,6 +500,30 @@ public void shouldRejectInClauseInLet()
.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)
+ .addReturn(select)
Review Comment:
nit: Would it make sense to return something like `SELECT a.whatever` here instead of a standard SELECT? I guess that would ensure we're hitting the error because of the LET specifically (as there is a reference return)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909252468
##########
test/unit/org/apache/cassandra/cql3/statements/TransactionStatementTest.java:
##########
@@ -483,6 +500,30 @@ public void shouldRejectInClauseInLet()
.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)
+ .addReturn(select)
Review Comment:
switched to `a.v`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1909848225
##########
test/unit/org/apache/cassandra/cql3/ast/Mutation.java:
##########
@@ -18,394 +18,794 @@
package org.apache.cassandra.cql3.ast;
-import java.nio.ByteBuffer;
import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Iterator;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
-import java.util.Map;
import java.util.Optional;
-import java.util.OptionalInt;
-import java.util.OptionalLong;
import java.util.Set;
-import java.util.stream.Collectors;
import java.util.stream.Stream;
-import java.util.stream.StreamSupport;
-
-import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
+import javax.annotation.Nullable;
import org.apache.cassandra.db.marshal.Int32Type;
import org.apache.cassandra.schema.ColumnMetadata;
import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.utils.ASTGenerators;
-import static org.apache.cassandra.cql3.ast.Elements.newLine;
-
-public class Mutation implements Statement
+public abstract class Mutation implements Statement
{
public enum Kind
{INSERT, UPDATE, DELETE}
public final Kind kind;
- public final TableMetadata table;
- public final Map<Symbol, Expression> values;
- private final Set<Symbol> partitionColumns, clusteringColumns, primaryColumns, nonPrimaryColumns;
- public final OptionalInt ttl;
- public final OptionalLong timestamp;
- public final Optional<? extends CasCondition> casCondition;
- private final List<Element> orderedElements = new ArrayList<>();
-
- public Mutation(Kind kind, TableMetadata table, Map<Symbol, Expression> values, OptionalInt ttl, OptionalLong timestamp, Optional<? extends CasCondition> casCondition)
- {
- this.kind = kind;
- this.table = table;
- this.values = values;
- this.ttl = ttl;
- this.timestamp = timestamp;
- this.casCondition = casCondition;
-
- // partition key is always required, so validate
- this.partitionColumns = toSet(table.partitionKeyColumns());
- this.clusteringColumns = toSet(table.clusteringColumns());
- this.primaryColumns = Sets.union(partitionColumns, clusteringColumns);
- Set<Symbol> allColumns = toSet(table.columns());
- nonPrimaryColumns = Sets.difference(allColumns, primaryColumns);
-
- Set<Symbol> requiredColumns;
- switch (kind)
- {
- case INSERT:
- case UPDATE:
- requiredColumns = primaryColumns;
- break;
- case DELETE:
- requiredColumns = partitionColumns;
- break;
- default:
- throw new IllegalArgumentException("Unknown kind: " + kind);
- }
- if (!values.keySet().containsAll(requiredColumns))
- throw new IllegalArgumentException("Not all required columns present; expected (" + requiredColumns + ") but was (" + values.keySet() + ")");
- }
+ public final TableReference table;
- private Mutation(Kind kind, TableMetadata table, Map<Symbol, Expression> values,
- Set<Symbol> partitionColumns, Set<Symbol> clusteringColumns, Set<Symbol> primaryColumns, Set<Symbol> nonPrimaryColumns,
- OptionalInt ttl, OptionalLong timestamp, Optional<? extends CasCondition> casCondition)
+ protected Mutation(Kind kind, TableReference table)
{
this.kind = kind;
this.table = table;
- this.values = values;
- this.partitionColumns = partitionColumns;
- this.clusteringColumns = clusteringColumns;
- this.primaryColumns = primaryColumns;
- this.nonPrimaryColumns = nonPrimaryColumns;
- this.ttl = ttl;
- this.timestamp = timestamp;
- this.casCondition = casCondition;
}
- public static Builder builder(TableMetadata table)
+ public abstract boolean isCas();
+
+ public final Mutation.Kind mutationKind()
{
- return new Builder(table);
+ return kind;
}
@Override
- public Statement.Kind kind()
+ public final Statement.Kind kind()
{
return Statement.Kind.MUTATION;
}
- public Mutation withoutTimestamp()
+ @Override
+ public String toString()
{
- return new Mutation(kind, table, values, partitionColumns, clusteringColumns, primaryColumns, nonPrimaryColumns, ttl, OptionalLong.empty(), casCondition);
+ return detailedToString();
}
- public Mutation withoutTTL()
+ public static InsertBuilder insert(TableMetadata metadata)
{
- return new Mutation(kind, table, values, partitionColumns, clusteringColumns, primaryColumns, nonPrimaryColumns, OptionalInt.empty(), timestamp, casCondition);
+ return new InsertBuilder(metadata);
}
- public Mutation withTable(TableMetadata table)
+ public static UpdateBuilder update(TableMetadata metadata)
{
- return new Mutation(kind, table, values, partitionColumns, clusteringColumns, primaryColumns, nonPrimaryColumns, ttl, timestamp, casCondition);
+ return new UpdateBuilder(metadata);
}
- public Map<Symbol, Expression> partitionKeys()
+ public static DeleteBuilder delete(TableMetadata metadata)
{
- return Maps.filterKeys(values, partitionColumns::contains);
+ return new DeleteBuilder(metadata);
}
- public Map<Symbol, Expression> clusteringKeys()
+ public static LinkedHashSet<Symbol> toSet(Iterable<ColumnMetadata> columns)
{
- return Maps.filterKeys(values, clusteringColumns::contains);
+ LinkedHashSet<Symbol> set = new LinkedHashSet<>();
+ for (var col : columns)
+ set.add(Symbol.from(col));
+ return set;
}
- public Map<Symbol, Expression> primaryKeys()
+ private static void toCQLWhere(Conditional conditional, StringBuilder sb, CQLFormatter formatter)
{
- return Maps.filterKeys(values, primaryColumns::contains);
+ formatter.section(sb);
+ sb.append("WHERE ");
+ List<Conditional> where = conditional.simplify();
+ formatter.group(sb);
+ for (var c : where)
+ {
+ formatter.element(sb);
+ c.toCQL(sb, formatter);
+ sb.append(" AND ");
+ }
+ sb.setLength(sb.length() - 5); // 5 = " AND "
+ formatter.endgroup(sb);
}
- public Object[] toRow()
+ public static class TTL implements Element
{
- //TODO (coverage): mutations don't really allow functions in the value def side... so this hack kinda works...
- Object[] array = new Object[table.columns().size()];
- Iterator<ColumnMetadata> it = table.allColumnsInSelectOrder();
- for (int i = 0; it.hasNext(); i++)
+ public final Value value;
+
+ public TTL(Value value)
{
- Symbol symbol = new Symbol(it.next());
- Expression expression = values.get(symbol);
- Optional<Object> opt = ExpressionEvaluator.tryEval(expression);
- array[i] = opt.orElseThrow(() -> new IllegalStateException("Unable to eval expression " + expression + "; " + expression.toCQL()));
+ this.value = value;
}
- return array;
- }
- public ByteBuffer[] toRowEncoded()
- {
- //TODO (coverage): mutations don't really allow functions in the value def side... so this hack kinda works...
- ByteBuffer[] array = new ByteBuffer[table.columns().size()];
- Iterator<ColumnMetadata> it = table.allColumnsInSelectOrder();
- for (int i = 0; it.hasNext(); i++)
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
{
- Symbol symbol = new Symbol(it.next());
- Expression expression = values.get(symbol);
- Optional<ByteBuffer> opt = ExpressionEvaluator.tryEvalEncoded(expression);
- array[i] = opt.orElseThrow(() -> new IllegalStateException("Unable to eval expression " + expression + "; " + expression.toCQL()));
+ sb.append("TTL ");
+ value.toCQL(sb, formatter);
}
- return array;
- }
- public boolean isCas()
- {
- return casCondition.isPresent();
+ @Override
+ public Stream<? extends Element> stream()
+ {
+ return Stream.of(value);
+ }
}
- @Override
- public void toCQL(StringBuilder sb, int indent)
+ public static class Timestamp implements Element
{
- if (!orderedElements.isEmpty())
- orderedElements.clear();
- switch (kind)
- {
- case INSERT:
- toCQLInsert(sb, indent);
- break;
- case UPDATE:
- toCQLUpdate(sb, indent);
- break;
- case DELETE:
- toCQLDelete(sb, indent);
- break;
- default:
- throw new IllegalArgumentException("Unsupported kind: " + kind);
+ public final Value value;
+
+ public Timestamp(Value value)
+ {
+ this.value = value;
}
- }
- private void toCQLInsert(StringBuilder sb, int indent)
- {
- /*
-INSERT INTO [keyspace_name.] table_name (column_list)
-VALUES (column_values)
-[IF NOT EXISTS]
-[USING TTL seconds | TIMESTAMP epoch_in_microseconds]
- */
- sb.append("INSERT INTO ").append(table.toString()).append(" (");
- List<Symbol> columnOrder = new ArrayList<>(values.keySet());
- for (Symbol name : columnOrder)
- {
- name.toCQL(sb, indent);
- orderedElements.add(name);
- sb.append(", ");
- }
- sb.setLength(sb.length() - 2);
- sb.append(")");
- newLine(sb, indent);
- sb.append("VALUES (");
- for (Symbol name : columnOrder)
- {
- Element value = values.get(name);
- orderedElements.add(value);
- value.toCQL(sb, indent);
- sb.append(", ");
- }
- sb.setLength(sb.length() - 2);
- sb.append(")");
- maybeAddCasCondition(sb, indent);
- maybeAddTTL(sb, indent);
- }
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
+ {
+ sb.append("TIMESTAMP ");
+ value.toCQL(sb, formatter);
+ }
- private void maybeAddCasCondition(StringBuilder sb, int indent)
- {
- if (casCondition.isPresent())
+ @Override
+ public Stream<? extends Element> stream()
{
- newLine(sb, indent);
- CasCondition condition = casCondition.get();
- orderedElements.add(condition);
- condition.toCQL(sb, indent);
+ return Stream.of(value);
}
}
- private void maybeAddTTL(StringBuilder sb, int indent)
+ public static class Using implements Element
{
- if (ttl.isPresent() || timestamp.isPresent())
+ public final Optional<TTL> ttl;
+ public final Optional<Timestamp> timestamp;
+
+ public Using(Optional<TTL> ttl, Optional<Timestamp> timestamp)
+ {
+ this.ttl = ttl;
+ this.timestamp = timestamp;
+ }
+
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
{
- newLine(sb, indent);
+ if (ttl.isEmpty() && timestamp.isEmpty())
+ return;
sb.append("USING ");
if (ttl.isPresent())
- sb.append("TTL ").append(ttl.getAsInt()).append(" ");
+ ttl.get().toCQL(sb, formatter);
if (timestamp.isPresent())
- sb.append(ttl.isPresent() ? "AND " : "").append("TIMESTAMP ").append(timestamp.getAsLong());
+ {
+ if (ttl.isPresent())
+ sb.append(" AND ");
+ timestamp.get().toCQL(sb, formatter);
+ }
}
}
- private void toCQLUpdate(StringBuilder sb, int indent)
+ public static class Insert extends Mutation
{
- /*
-UPDATE [keyspace_name.] table_name
-[USING TTL time_value | USING TIMESTAMP timestamp_value]
-SET assignment [, assignment] . . .
-WHERE row_specification
-[IF EXISTS | IF condition [AND condition] . . .] ;
- */
- sb.append("UPDATE ").append(table.toString());
- maybeAddTTL(sb, indent);
- newLine(sb, indent);
- sb.append("SET ");
- int subindent = indent + 4;
- for (Symbol name : nonPrimaryColumns)
- {
- newLine(sb, subindent);
- Element value = values.get(name);
- name.toCQL(sb, subindent);
- orderedElements.add(name);
- // when a AssignmentOperator the `=` is added there so don't add
- //TODO this is super hacky...
- if (!(value instanceof AssignmentOperator))
- sb.append('=');
- value.toCQL(sb, subindent);
- orderedElements.add(value);
- sb.append(", ");
- }
- if (!nonPrimaryColumns.isEmpty())
- sb.setLength(sb.length() - 2);
- newLine(sb, indent);
- sb.append("WHERE ");
- valuesAnd(sb, indent + 2, primaryColumns);
- maybeAddCasCondition(sb, indent);
+ public final LinkedHashMap<Symbol, Expression> values;
+ public final boolean ifNotExists;
+ public final Optional<Using> using;
+
+ public Insert(TableReference table, LinkedHashMap<Symbol, Expression> values, boolean ifNotExists, Optional<Using> using)
+ {
+ super(Mutation.Kind.INSERT, table);
+ this.values = values;
+ this.ifNotExists = ifNotExists;
+ this.using = using;
+ }
+
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
+ {
+ // INSERT INTO [keyspace_name.] table_name (column_list)
+ sb.append("INSERT INTO ");
+ table.toCQL(sb, formatter);
+ sb.append(" (");
+ for (Symbol col : values.keySet())
+ {
+ col.toCQL(sb, formatter);
+ sb.append(", ");
+ }
+ sb.setLength(sb.length() - 2); // 2 = ", "
+ sb.append(")");
+ // VALUES (column_values)
+ formatter.section(sb);
+ sb.append("VALUES (");
+ for (Expression e : values.values())
+ {
+ e.toCQL(sb, formatter);
+ sb.append(", ");
+ }
+ sb.setLength(sb.length() - 2); // 2 = ", "
+ sb.append(")");
+ // [IF NOT EXISTS]
+ if (ifNotExists)
+ {
+ formatter.section(sb);
+ sb.append("IF NOT EXISTS");
+ }
+ // [USING TTL seconds | TIMESTAMP epoch_in_microseconds]
+ if (using.isPresent())
+ {
+ formatter.section(sb);
+ using.get().toCQL(sb, formatter);
+ }
+ }
+
+ @Override
+ public Stream<? extends Element> stream()
+ {
+ List<Element> elements = new ArrayList<>(1 + (using.isPresent() ? 1 : 0) + (values.size() * 2));
+ elements.add(table);
+ values.keySet().forEach(elements::add);
+ values.values().forEach(elements::add);
Review Comment:
nit: These could just be...
```
elements.addAll(values.keySet());
elements.addAll(values.values());
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892963534
##########
test/harry/main/org/apache/cassandra/harry/cql/SelectHelper.java:
##########
@@ -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())),
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892964144
##########
test/harry/main/org/apache/cassandra/harry/cql/SelectHelper.java:
##########
@@ -146,7 +145,7 @@ public static CompiledStatement select(Operations.SelectCustom select, SchemaSpe
}
}
- builder.allowFiltering();
+ builder.withAllowfiltering();
Review Comment:
`allowFiltering` was the only method in the class that wasn't prefixed with `with`, so migrated to match the style of the class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893002393
##########
test/unit/org/apache/cassandra/cql3/ast/Conditional.java:
##########
@@ -34,32 +41,171 @@ default AbstractType<?> type()
return BooleanType.instance;
}
+ @Override
+ default Conditional visit(Visitor v)
+ {
+ return v.visit(this);
+ }
+
+ default List<Conditional> simplify()
+ {
+ return Collections.singletonList(this);
+ }
+
+ class Where implements Conditional
+ {
+ public enum Inequality
+ {
+ EQUAL("="),
+ NOT_EQUAL("!="),
+ GREATER_THAN(">"),
+ GREATER_THAN_EQ(">="),
+ LESS_THAN("<"),
+ LESS_THAN_EQ("<=");
+
+ private final String value;
+
+ Inequality(String value)
+ {
+ this.value = value;
+ }
+ }
+
+ public final Inequality kind;
+ public final Expression lhs;
+ public final Expression rhs;
+
+ private Where(Inequality kind, Expression lhs, Expression rhs)
+ {
+ this.kind = kind;
+ this.lhs = lhs;
+ this.rhs = rhs;
+ }
+
+ public static Where create(Inequality kind, Expression ref, Expression expression)
+ {
+ return new Where(kind, ref, expression);
+ }
+
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
+ {
+ lhs.toCQL(sb, formatter);
+ sb.append(' ').append(kind.value).append(' ');
+ rhs.toCQL(sb, formatter);
+ }
+
+ @Override
+ public Stream<? extends Element> stream()
+ {
+ return Stream.of(lhs, rhs);
+ }
+
+ @Override
+ public Conditional visit(Visitor v)
+ {
+ var u = v.visit(this);
+ if (u != this) return u;
+ var lhs = this.lhs.visit(v);
+ var rhs = this.rhs.visit(v);
+ if (lhs == this.lhs && rhs == this.rhs) return this;
+ return new Where(kind, lhs, rhs);
+ }
+ }
+
+ class Between implements Conditional
+ {
+ public final Expression ref, start, end;
+
+ public Between(Expression ref, Expression start, Expression end)
+ {
+ this.ref = ref;
+ this.start = start;
+ this.end = end;
+ }
+
+ @Override
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
+ {
+ ref.toCQL(sb, formatter);
+ sb.append(" BETWEEN ");
+ start.toCQL(sb, formatter);
+ sb.append(" AND ");
+ end.toCQL(sb, formatter);
+ }
+
+ @Override
+ public Stream<? extends Element> stream()
+ {
+ return Stream.of(ref, start, end);
+ }
+
+ @Override
+ public Conditional visit(Visitor v)
+ {
+ var u = v.visit(this);
+ if (u != this) return u;
+ Expression ref = this.ref.visit(v);
+ Expression start = this.start.visit(v);
+ Expression end = this.end.visit(v);
+ if (ref == this.ref && start == this.start && end == this.end) return this;
+ return new Between(ref, start, end);
+ }
+ }
+
class In implements Conditional
{
- public final ReferenceExpression symbol;
+ public final Expression symbol;
public final List<Expression> expressions;
- public In(ReferenceExpression symbol, List<Expression> expressions)
+ public In(Expression symbol, List<Expression> expressions)
{
Invariants.checkArgument(!expressions.isEmpty());
this.symbol = symbol;
this.expressions = expressions;
}
@Override
- public void toCQL(StringBuilder sb, int indent)
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
{
- symbol.toCQL(sb, indent);
+ symbol.toCQL(sb, formatter);
sb.append(" IN ");
sb.append('(');
for (Expression e : expressions)
{
- e.toCQL(sb, indent);
+ e.toCQL(sb, formatter);
sb.append(", ");
}
sb.setLength(sb.length() - 2); // ", "
sb.append(')');
}
+
+ @Override
+ public Stream<? extends Element> stream()
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893002763
##########
test/unit/org/apache/cassandra/cql3/ast/Conditional.java:
##########
@@ -77,29 +223,158 @@ public enum Kind
}
}
+ public final ReferenceExpression reference;
public final Kind kind;
- public final Reference reference;
public Is(String symbol, Kind kind)
{
- this(Reference.of(Symbol.unknownType(symbol)), kind);
+ this(Symbol.unknownType(symbol), kind);
}
- public Is(Reference reference, Kind kind)
+ public Is(ReferenceExpression reference, Kind kind)
{
- this.kind = kind;
this.reference = reference;
+ this.kind = kind;
}
@Override
- public void toCQL(StringBuilder sb, int indent)
+ public void toCQL(StringBuilder sb, CQLFormatter formatter)
{
- reference.toCQL(sb, indent);
+ reference.toCQL(sb, formatter);
sb.append(" IS ").append(kind.cql);
}
}
- class Builder
+ class And implements Conditional
Review Comment:
moved from its own class to here to be consistent with the other types in this package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1892982775
##########
test/unit/org/apache/cassandra/cql3/ast/CQLFormatterPrettyPrintTest.java:
##########
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.cql3.ast;
+
+import org.junit.Test;
+
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.schema.TableMetadata;
+import org.assertj.core.api.Assertions;
+
+public class CQLFormatterPrettyPrintTest
+{
+ private static TableMetadata TBL1 = TableMetadata.builder("ks", "tbl")
+ .addPartitionKeyColumn("pk", Int32Type.instance)
+ .addClusteringColumn("ck", Int32Type.instance)
+ .addRegularColumn("v0", Int32Type.instance)
+ .addRegularColumn("v1", Int32Type.instance)
+ .build();
+ private static final Symbol pk = new Symbol("pk", Int32Type.instance);
+ private static final Symbol ck = new Symbol("ck", Int32Type.instance);
Review Comment:
these are dead code... I just wanted to be complete...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893008583
##########
test/unit/org/apache/cassandra/cql3/ast/FunctionCall.java:
##########
@@ -87,6 +107,16 @@ public static FunctionCall count(String symbol)
public static FunctionCall count(Symbol symbol)
{
- return new FunctionCall("count", Collections.singletonList(Reference.of(symbol)), LongType.instance);
+ return new FunctionCall("count", Collections.singletonList(symbol), LongType.instance);
+ }
+
+ 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);
Review Comment:
dead code in this patch, but used in the follow up model patch. This is here for the following
```
WHERE token(pk) = token(?)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20155: To improve accord interoperability test coverage, nehttps://github.com/dcapwell/cassandra/pull/new/CASSANDRA-20155 [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3753:
URL: https://github.com/apache/cassandra/pull/3753#discussion_r1893007222
##########
test/unit/org/apache/cassandra/cql3/ast/Element.java:
##########
@@ -22,17 +22,24 @@
public interface Element
{
- void toCQL(StringBuilder sb, int indent);
+ void toCQL(StringBuilder sb, CQLFormatter formatter);
default void toCQL(StringBuilder sb)
{
- toCQL(sb, 0);
+ toCQL(sb, CQLFormatter.None.instance);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org