CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types #3995

dcapwell · 2025-03-19T21:14:45Z

No description provided.

dcapwell · 2025-03-19T21:28:54Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

+        return AbstractTypeGenerators.builder()
+                                     .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+                                     .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))
+                                     .withUserTypeFields(AbstractTypeGenerators.UserTypeFieldsGen.simpleNames())


not testing UDT serialization so simple names makes it so much easier to read...

dcapwell · 2025-03-19T21:29:57Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

+                                     .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+                                     .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))


before we used uniform random so each test had the same weights. Now we select a new distribution for each test run (some tests will only be primitives, others will be mostly maps, etc.)

dcapwell · 2025-03-19T21:30:38Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

+                                     .withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
+                                     .withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))
+                                     .withUserTypeFields(AbstractTypeGenerators.UserTypeFieldsGen.simpleNames())
+                                     .withMaxDepth(1);


ATM we are not testing nested access (will need to eventually) so lowered as the cql literal output was so large it got hard to read (UDT of map of UDT of map of UDT....)

dcapwell · 2025-03-19T21:31:13Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

@@ -206,7 +229,7 @@ protected List<CreateIndexDDL.Indexer> supportedIndexers()
            builder.value(pk, key.bufferAt(pks.indexOf(pk)));


-        List<Symbol> searchableColumns = state.nonPartitionColumns;
+        List<Symbol> searchableColumns = state.searchableNonPartitionColumns;


multi cell collections/udt can not be searched, so just using the non-partition columns wasn't enough

dcapwell · 2025-03-19T21:32:23Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

@@ -363,7 +386,8 @@ protected TableMetadata defineTable(RandomSource rs, String ks)
                                       .withCompression())
                     .withKeyspaceName(ks).withTableName("tbl")
                     .withSimpleColumnNames()
-                     .withDefaultTypeGen(supportedTypes())
+                     .withDefaultTypeGen(supportedTypes(rs))
+                     .withPrimaryColumnTypeGen(supportedPrimaryColumnTypes(rs))


when primary keys can be collections there can be issues with having too many bytes and having our mutations be rejected. Keeping it primitive also improves readability for debug output...

Would be good to have a test that overrides this though, just to improve coverage...

dcapwell · 2025-03-19T21:33:48Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
                                                                  .withoutTransaction()
                                                                  .withoutTtl()
                                                                  .withoutTimestamp()
-                                                                  .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+                                                                  .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+                                                                  .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));


each column will have a different distribution for when operators are allowed

Required for CASSANDRA-20449 repro at least 😄

wasn't required; we hit the issue before this change. This change is to better cover different edge cases

We need different assignment operators to repro (+= v vs. = {v}) but not necessarily different distributions of operators, which is what's introduced here - is that right?

dcapwell · 2025-03-19T22:09:51Z

test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java

@@ -493,6 +503,45 @@ private static void validateAnyOrder(ImmutableUniqueList<Symbol> columns, Set<Ro
            if (actual.isEmpty()) sb.append("No rows returned");
            else sb.append("Missing rows:\n").append(table(columns, missing));
        }
+        if (!unexpected.isEmpty() && unexpected.size() == missing.size())


this is extra human readable validation logic. null vs <empty> has been super hard to see from the above output, and the use case i was finding is that we had the correct rows returned, but 1 or more columns had a difference... so if this is the case we try to show which columns are off... this was super useful for detecting consistency issues with null and <empty>

dcapwell · 2025-03-19T22:10:12Z

test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java

+        {
+            // good chance a column differs
+            StringBuilder finalSb = sb;
+            Runnable runOnce = new Runnable()


i think Alex created a reusable function for this in accord... would need to find it

dcapwell · 2025-03-19T22:21:10Z

test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java

@@ -731,7 +780,7 @@ private static boolean matches(ByteBuffer value, List<? extends Expression> cond
        for (Expression e : conditions)
        {
            ByteBuffer expected = eval(e);
-            if (expected.equals(value))
+            if (expected != null && expected.equals(value))


eval is now Nullable but this code path shouldn't really ever see a null... maybe if you did something like [1] - [1] then we could get a null here... mostly added to be correct and defensive (and intellij yelled at me)

test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java

dcapwell · 2025-03-19T23:21:12Z

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

@@ -141,7 +141,7 @@ else if (((isFrozen(type) && !type.isVector()) || StorageAttachedIndex.SUPPORTED
        public EnumSet<QueryType> supportedQueries(AbstractType<?> type)
        {
            type = type.unwrap();
-            if (IndexTermType.isEqOnlyType(type))
+            if (IndexTermType.isEqOnlyType(type) || type.isCollection() || type.isUDT() || type.isTuple())


@maedhroz should we push this into IndexTermType.isEqOnlyType?

dcapwell · 2025-03-19T23:22:32Z

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


 public class ExpressionEvaluator
 {
-    public static Optional<Object> tryEval(Expression e)
+    @Nullable
+    public static Object eval(Expression e)


returning Optional was just causing confusion, and all code paths did .get() anyways.

I also needed the ability to return null rather than empty which i couldn't do with Optional

dcapwell · 2025-03-19T23:23:13Z

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

    {
+        Object lhs = eval(e.left);


the model eval method will put the "current" value as a ByteBuffer into the operation, this then means that the types might not match! to solve this we just convert w/e we see a BB.

dcapwell · 2025-03-19T23:23:42Z

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

+                    accum.removeAll((List<Object>) rhs);
+                    return accum.isEmpty() ? null : accum;
+                }
+                if (lhs instanceof Map)


this code path is currently untested, only part of this patch that wasn't validated

dcapwell · 2025-03-19T23:24:55Z

test/unit/org/apache/cassandra/utils/ASTGenerators.java

@@ -770,12 +784,12 @@ private void generateRemaining(RandomnessSource rnd,
                    }
                }
            }
-            if (kind == Mutation.Kind.UPDATE && isTransaction)


collections can do things outside of transactions, so need to push this into supportsOperators

dcapwell · 2025-03-19T23:26:02Z

test/unit/org/apache/cassandra/utils/AbstractTypeGenerators.java

            typeFilter = other.typeFilter;
+            udtName = other.udtName;
+            multiCellGen = other.multiCellGen;


this was missing so i took the fields in object order and recreated this constructor just in case there was more; looks like it was just this one though.

dcapwell · 2025-03-19T23:27:59Z

test/unit/org/apache/cassandra/utils/Generators.java

+            for (int i = 0; i < size; i++)
+            {
+                T value;
+                while (!set.add(value = gen.generate(rnd))) {}


this isn't as safe as uniqueBestEffort from accord (if gen is a boolean gen and size is 3, this will loop forever)... once accord merges we can talk about unifying all this stuff...

In the meantime, want to set an iteration limit with a specific error? Just to make it a bit easier to figure out what's happening if a test is timing out.

for now no. There are better impl in Gens and when i migrate off of quick theories we can just use the better versions; so no reason to handle here...

bbotella · 2025-03-20T17:16:47Z

test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java

+    {
+        if (!(e instanceof AssignmentOperator)) return eval(e);
+        // multi cell collections have the property that they do update even if the current value is null
+        boolean isFancy = col.type().isCollection() && col.type().isMultiCell();


Curious about the isFancy name choosing :-)

i felt it was fancy!

bbotella · 2025-03-20T17:20:11Z

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

+                return EnumSet.of(Kind.ADD, Kind.SUBTRACT);
+            if (type instanceof MapType)
+            {
+                // map supports subtract, but not map - map; only map - set!


That's... an interesting finding. Should we have a Jira to add proper support?

i wouldn't add a JIRA unless someone wants that feature. map -= acts on the key level, which can be done without a transaction (CAS or Accord), where as remove(key, value) would need to know the current value, which isn't behavior we like to support outside of transactions.

…anged per run

…ir elements

…filtering

… the same size, so can special case this to produce an easier to understand diff of what had a mismatch. now <empty> is no longer null in toString, its <empty> so its easier to detect <empty> != null mismatch

…y index contains rather than isSearchable

dcapwell · 2025-03-20T23:30:13Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

@@ -523,6 +561,8 @@ private List<Symbol> multiColumnQueryColumns()
            List<Symbol> allowedColumns = searchableColumns;
            if (hasMultiNodeMultiColumnAllowFilteringWithLocalWritesIssue())
                allowedColumns = nonPkIndexedColumns;
+            if (IGNORED_ISSUES.contains(KnownIssue.SAI_AND_VECTOR_COLUMNS) && !indexes.isEmpty())


added to get clean CI. Something is up with multi column queries that touch both SAI and vector type. It doesn't look like the vector must be indexed to get incorrect results, so doesn't feel like we are selecting a bad index here

This reverts commit 4dda3e6.

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

aratno · 2025-03-21T14:11:45Z

test/unit/org/apache/cassandra/utils/Generators.java

+            for (int i = 0; i < size; i++)
+            {
+                T value;
+                while (!set.add(value = gen.generate(rnd))) {}


In the meantime, want to set an iteration limit with a specific error? Just to make it a bit easier to figure out what's happening if a test is timing out.

test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java

aratno · 2025-03-21T15:05:24Z

test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java

@@ -424,7 +448,8 @@ public State(RandomSource rs, Cluster cluster)
                                                                  .withoutTransaction()
                                                                  .withoutTtl()
                                                                  .withoutTimestamp()
-                                                                  .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions));
+                                                                  .withPartitions(SourceDSL.arbitrary().pick(uniquePartitions))
+                                                                  .withColumnExpressions(e -> e.withOperators(Generators.fromGen(BOOLEAN_DISTRIBUTION.next(rs))));


Required for CASSANDRA-20449 repro at least 😄

dcapwell · 2025-03-21T18:58:45Z

merged

dcapwell requested a review from maedhroz March 19, 2025 21:14

dcapwell force-pushed the CASSANDRA-20460 branch from 8af3aa8 to f978444 Compare March 19, 2025 21:25

dcapwell commented Mar 19, 2025

View reviewed changes

test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java Show resolved Hide resolved

dcapwell commented Mar 19, 2025

View reviewed changes

bbotella reviewed Mar 20, 2025

View reviewed changes

dcapwell added 10 commits March 20, 2025 14:54

when UDT sees empty bytes this becomes null

21c4f30

checkpoint of current test finding missing insert

d1be462

fixed another model bug with UDTs and empty bytes

7982756

need to set null rather than unset so the cell value is updated

e99535a

first draft of assignment operator support in the model

76dfa34

fixed NPE when c -= returns null

f5a59c2

fixed bug where update static didnt handle assignment operator

75143d2

added set update support

9f7d6a3

added support for set - set

3eb4735

added map support

289334d

dcapwell added 17 commits March 20, 2025 14:54

each column gets its own boolean distro for allowing assignment or not

e2fb393

made type kind and what primitives to use based off a distribution ch…

aaa083d

…anged per run

fixed a bug where frozen collections would allow range

2c994c9

fixed bugs related to trying to do search collections rather than the…

961f618

…ir elements

clenaup

7915895

fixed bug where we tried to do filtering on UDTs

b93943d

fixed bug where we would try to do < queries on tuples without allow …

42588bd

…filtering

when only a subset of columns differ than missing and unexpected have…

202ba7a

… the same size, so can special case this to produce an easier to understand diff of what had a mismatch. now <empty> is no longer null in toString, its <empty> so its easier to detect <empty> != null mismatch

when only a subset of columns differ than missing and unexpected have…

74e084a

… the same size, so can special case this to produce an easier to understand diff of what had a mismatch. now <empty> is no longer null in toString, its <empty> so its easier to detect <empty> != null mismatch

fixed a bug where frozen udts would store null rather than <empty>

b76f2ba

checkpoint failing seed

639427b

revert

2ba8dd0

cleanup for pr

e475980

made partitions non-uniform. fixed bug where serachable was defined b…

ccc8a34

…y index contains rather than isSearchable

cleanup and npe

ee6c718

cleanup

171c41a

cleanup

9eb86e7

dcapwell force-pushed the CASSANDRA-20460 branch from b9aa139 to 9eb86e7 Compare March 20, 2025 21:54

dcapwell added 2 commits March 20, 2025 16:22

testing out removing vectors from SAI search

5de3604

revert while true loop

fbbdfdd

dcapwell commented Mar 20, 2025

View reviewed changes

dcapwell added 2 commits March 20, 2025 16:38

added unset to cql

4dda3e6

Revert "added unset to cql"

44a3f1c

This reverts commit 4dda3e6.

aratno approved these changes Mar 21, 2025

View reviewed changes

dcapwell added 3 commits March 21, 2025 11:36

added unit test for += and -=

3e1d786

feedback

c03ccde

feedback

34af875

dcapwell closed this Mar 21, 2025

dcapwell deleted the CASSANDRA-20460 branch March 21, 2025 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types #3995

CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types #3995

dcapwell commented Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

aratno Mar 21, 2025

dcapwell Mar 21, 2025

aratno Mar 21, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025 •

edited

Loading

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025

dcapwell Mar 19, 2025 •

edited

Loading

aratno Mar 21, 2025

dcapwell Mar 21, 2025

bbotella Mar 20, 2025

dcapwell Mar 20, 2025

bbotella Mar 20, 2025

dcapwell Mar 20, 2025

dcapwell Mar 20, 2025

aratno Mar 21, 2025

aratno Mar 21, 2025

dcapwell commented Mar 21, 2025

		.withTypeKinds(Generators.fromGen(TYPE_KIND_DISTRIBUTION.next(rs)))
		.withPrimitives(Generators.fromGen(PRIMITIVE_DISTRIBUTION.next(rs)))

CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types #3995

CASSANDRA-20460: Expand TableWalk tests to include collections and add support for += and -= for these types #3995

Conversation

dcapwell commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell commented Mar 21, 2025

dcapwell Mar 19, 2025 •

edited

Loading

dcapwell Mar 19, 2025 •

edited

Loading