To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states #3785

dcapwell · 2025-01-13T18:26:35Z

No description provided.

dcapwell · 2025-01-13T18:28:28Z

src/java/org/apache/cassandra/db/filter/RowFilter.java

@@ -536,7 +536,7 @@ protected ByteBuffer getValue(TableMetadata metadata, DecoratedKey partitionKey,
                    return row.clustering().bufferAt(column.position());
                default:
                    Cell<?> cell = row.getCell(column);
-                    return cell == null ? null : cell.buffer();
+                    return cell == null || cell.isTombstone() || !cell.isLive(nowInSec) ? null : cell.buffer();


backport

commit 4fc8bb29fcda935728d8863a4499fa0e9d924b82 Author: Sunil Ramchandra Pawar <[email protected]> Date: Tue Jan 7 08:58:48 2025 -0800 IndexOutOfBoundsException when accessing partition where the column was deleted patch by Sunil Ramchandra Pawar; reviewed by Caleb Rackliffe, David Capwell for CASSANDRA-20108

as these tests detect this issue

dcapwell · 2025-01-13T18:29:29Z

...ributed/org/apache/cassandra/distributed/test/cql3/AccordInteropSingleNodeTableWalkTest.java

+import accord.utils.Property;
+import org.apache.cassandra.service.consensus.TransactionalMode;
+
+public class AccordInteropSingleNodeTableWalkTest extends SingleNodeTableWalkTest


I think im going to drop all Interop tests for now as they are all broken... Ariel has a patch in-flight to fix known issues... so rather than blocking this patch its prob best to just skip testing accord for now...

dcapwell · 2025-01-13T18:34:04Z

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

@@ -223,7 +232,7 @@ private RowState updateRowState(RowState currentState, IntFunction<Bijections.Bi

                assert lts >= currentState.lts[i] : String.format("Out-of-order LTS: %d. Max seen: %s", lts, currentState.lts[i]); // sanity check; we're iterating in lts order

-                if (currentState.lts[i] == lts)
+                if (lts != MagicConstants.NO_TIMESTAMP && currentState.lts[i] == lts)


the ast tests down track timestamps so this would always hit this branch causing test failures... needed to disable this logic when doing a write and just assume that the write is newer

"down track"?

Sorry, bad sentence.

the ast tests in this PR do not track timestamps, they rely on server to calculate them. The existing logic expects harry to own the timestamps and update the lts, so i need a way to opt-out of this behavior; hence this change.

When the caller defines the lts as NO_TIMESTAMP that tells harry to just accept the write and override, rather than try to deal with timestamp conflicts

dcapwell · 2025-01-13T18:34:39Z

test/unit/org/apache/cassandra/cql3/validation/operations/SelectTest.java

@@ -2554,6 +2554,36 @@ public void filteringOnStaticColumnTest() throws Throwable
        });
    }

+    @Test
+    public void filteringOnDeletedStaticColumnValue() throws Throwable


part of

commit 4fc8bb29fcda935728d8863a4499fa0e9d924b82 Author: Sunil Ramchandra Pawar <[email protected]> Date: Tue Jan 7 08:58:48 2025 -0800 IndexOutOfBoundsException when accessing partition where the column was deleted patch by Sunil Ramchandra Pawar; reviewed by Caleb Rackliffe, David Capwell for CASSANDRA-20108

dcapwell · 2025-01-13T18:36:37Z

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

@@ -45,4 +55,42 @@ public void dnsDomainName()
    {
        qt().forAll(Generators.DNS_DOMAIN_NAME).checkAssert(InternetDomainName::from);
    }
+
+    @Test
+    public void asciiDeterministic()


I thought I found a deterministic issue with text but it ended up being an issue with IntelliJ displaying texts... these tests didn't solve any problem but did tell me that could be the issue...

by default text types now display a descriptor rather than the raw text, and lets you override and show the text with a flag... one issue is that the text displayed by IntelliJ might not be the exact string which can make repos harder....

i think i finally found the issue... when the utf-8 strings have control chars this can cause intellij to produce bad output... I added logic to convert those to escaped chars so the string output is safe. This also makes it more clear what chars are present as could be anything

dcapwell · 2025-01-13T18:38:27Z

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

+
+import org.agrona.collections.Object2IntHashMap;
+
+public class ImmutableUniqueList<T> extends AbstractList<T> implements RandomAccess


java doesn't allow a List to also be a Set as they have method conflicts... so this List has the same properties of a Set but also maintain the insertion order dependent properties of List.

LinkedHashSet wasn't enough as the accesses pattern of index -> Value was cumbersome...

dcapwell · 2025-01-22T23:14:37Z

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

+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.service.consensus.TransactionalMode;
+
+public class MultiNodeTableWalkTest extends SingleNodeTableWalkTest


this test is currently not stable.... filed 3 tickets last night due to this test...

aweisberg

Nothing really earth shattering found.

test/distributed/org/apache/cassandra/distributed/shared/ClusterUtils.java

aweisberg · 2025-01-22T21:42:34Z

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

+            else
+            {
+                NavigableSet<Clustering<ByteBuffer>> clusteringKeys = partition.clusteringKeys();
+                Clustering<ByteBuffer> clusteringKey = rs.pickOrderedSet(clusteringKeys);


Ranges of clustering keys or is that not a priority RN because Accord doesn't interact with clustering keys much?

this isn't ranges. We have selected a partition we wish to query, we look at all the clustering keys (aka rows) present in the partition, and we select one of them.

The goal of this else block is to pick a "row" to query

Oh my suggestion was to add ranges of clustering keys, not implying that it currently does that. We can call it out of scope for this PR.

we actually had range support, and between... i dropped from this PR but its in #3844

the big issue is the existence of bugs on trunk.

Ill leave a TODO to add partition restricted clustering key filter command... WHERE pk=? and ck BETWEEN ? AND ?

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

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

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

aweisberg · 2025-01-23T19:44:46Z

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

@@ -223,7 +232,7 @@ private RowState updateRowState(RowState currentState, IntFunction<Bijections.Bi

                assert lts >= currentState.lts[i] : String.format("Out-of-order LTS: %d. Max seen: %s", lts, currentState.lts[i]); // sanity check; we're iterating in lts order

-                if (currentState.lts[i] == lts)
+                if (lts != MagicConstants.NO_TIMESTAMP && currentState.lts[i] == lts)


"down track"?

aweisberg

+1 TY only thing left is adding that comment like you said

aweisberg · 2025-01-31T20:00:57Z

test/distributed/org/apache/cassandra/distributed/test/JavaDriverUtils.java

@@ -77,4 +78,24 @@ public static List<InetSocketAddress> buildContactPoints(ICluster<? extends IIns
                    .map(ClusterUtils::getNativeInetSocketAddress)
                    .collect(Collectors.toList());
    }
+
+    public static ConsistencyLevel toDriverCL(org.apache.cassandra.distributed.api.ConsistencyLevel cl)


This is unused here?

not used anymore. This was moved out of StatefulASTBase. In trunk i have a test to repo a history, so had to refactor that out

aweisberg · 2025-01-31T20:07:04Z

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

            neededPks.addAll(partitionColumns);
        }

+        public static List<ColumnMetadata> safeColumns(TableMetadata metadata)


Change to allColumnsInSelectOrder?

you had the same comment with ASTGenerators.safeColumns... let me unify this stuff

pushed this to TableMetadata and removed this function and the one in ASTGenerators

aweisberg · 2025-01-31T20:09:59Z

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

+        }
+    }
+    protected static final EnumSet<KnownIssue> IGNORED_ISSUES = EnumSet.allOf(KnownIssue.class);
+    protected static boolean CQL_DEBUG_APPLY_OPERATOR = false;


What does this do? Does it evaluate expressions before logging them to simplify? Maybe add a comment.

ill add a comment... this is only something i understand =D

So, we generate stuff like 4 + 4 but when things fail you have the resolved value of 8. When you look at the history you can't use grep to filter for 8 as 4 + 4 will get filtered out... this option will change the history to "apply" or "resolve" the operator so it shows 8 instead.

pushed this doc

/** * mutations and selects will use operators (eg. {@code 4 + 4}, the + operator), and this will be reflected in the history output. * * When an issue is found its common to filter out insertions to different partitions/rows but this can become a problem * as the issue is for {@code pk=8} but the insert is to {@code 4 + 4}! * * Setting this to {@code true} will cause all operators to be "applied" or "executored" and the CQL in the history * will be the output (eg. {@code 4 + 4 } is replaced with {@code 8}). */ protected static boolean CQL_DEBUG_APPLY_OPERATOR = false;

…h track warnings

… and removed the ; in the annotate as it doesnt help

…as they are not stable until next rebase against trunk

…g, and token range search bug fix

dcapwell · 2025-02-01T03:45:45Z

merged

dcapwell mentioned this pull request Jan 13, 2025

CASSANDRA-20156: To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states dcapwell/cassandra#3

Closed

dcapwell commented Jan 13, 2025

View reviewed changes

dcapwell force-pushed the CASSANDRA-20156 branch from ec4d401 to d4639b2 Compare January 18, 2025 00:13

aweisberg self-requested a review January 22, 2025 21:00

dcapwell commented Jan 22, 2025

View reviewed changes

aweisberg requested changes Jan 23, 2025

View reviewed changes

dcapwell force-pushed the CASSANDRA-20156 branch from 87a06ed to 9ad2fc4 Compare January 25, 2025 00:47

belliottsmith force-pushed the cep-15-accord branch from 5bbc4ae to e0f6237 Compare January 27, 2025 21:00

aweisberg approved these changes Jan 29, 2025

View reviewed changes

dcapwell force-pushed the CASSANDRA-20156 branch from 3bd93d0 to 1f48fdd Compare January 30, 2025 00:52

aweisberg reviewed Jan 31, 2025

View reviewed changes

dcapwell added 12 commits January 31, 2025 13:19

Checkpointing a first draft of a AST Model

48d5e04

fixed backport

948c21f

fixed backport

66e5f22

checkpoint but need to give up, java is fighting mixed case too much

8a866d6

drop the inet test as it cant really do what is needed

d9911d0

bump accord

6e73fce

checking in multi expression work so i can focus on the bug found wit…

536e672

…h track warnings

made SHOW_REAL_VALUES something you can update in the preCheck method…

68bd294

… and removed the ; in the annotate as it doesnt help

checkpointing more failing seeds

54e1cb8

working on test cases

1545a6f

test the filtering logic that wasnt working last week

48d904c

cleanup seeds

20915fd

dcapwell added 22 commits January 31, 2025 13:19

multi node needs Feature.NETWORK

599bbe8

bump accord

31976e9

fixed issue with rebase

a7f8f1d

expose methods to make repo tests easier

b1bc6b4

added full/mixed_reads interop tests and removed the AF/SAI commands …

0a64fe1

…as they are not stable until next rebase against trunk

cleanup test so its only the passable cases

cf49e71

fixup

30ce1bf

pull forward some trunk stuff

b1fac98

lower diff with trunk

d772ce8

lower diff with trunk

373c17a

bring in the bug fixes for ast model

3a6ec76

inline

dabdcb4

brought in bug fixes from trunk, and the apply apperator improvement

70a558d

formatting

9043794

added comment and todo

e698ae4

import

f229acc

backport bug fixes from trunk... now select star from table is workin…

32aa970

…g, and token range search bug fix

got in-sync with trunk

35dc006

all accord tests now ignore journal issue

b83b2c9

added multi node token walker from trunk

de2871a

added token conflict accord tests for single node

3e30e7b

added multi node

57d9a38

dcapwell force-pushed the CASSANDRA-20156 branch from 7b4c49e to 57d9a38 Compare January 31, 2025 22:58

dcapwell added 4 commits January 31, 2025 14:59

fixed constructor

b4fa72b

mark abstract

28c60ac

unify column fixed order

52be582

document

beb197c

dcapwell closed this Feb 1, 2025

dcapwell deleted the CASSANDRA-20156 branch February 1, 2025 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states #3785

To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states #3785

dcapwell commented Jan 13, 2025

dcapwell Jan 13, 2025

dcapwell Jan 13, 2025

dcapwell Jan 13, 2025

aweisberg Jan 23, 2025

dcapwell Jan 23, 2025

dcapwell Jan 13, 2025

dcapwell Jan 13, 2025

dcapwell Jan 23, 2025

dcapwell Jan 13, 2025

dcapwell Jan 22, 2025

aweisberg left a comment

aweisberg Jan 22, 2025

dcapwell Jan 23, 2025

aweisberg Jan 29, 2025

dcapwell Jan 30, 2025

aweisberg Jan 23, 2025

aweisberg left a comment

aweisberg Jan 31, 2025

dcapwell Jan 31, 2025

aweisberg Jan 31, 2025

dcapwell Jan 31, 2025

dcapwell Jan 31, 2025

aweisberg Jan 31, 2025 •

edited

Loading

dcapwell Jan 31, 2025

dcapwell Jan 31, 2025

dcapwell commented Feb 1, 2025


		import org.agrona.collections.Object2IntHashMap;

		public class ImmutableUniqueList<T> extends AbstractList<T> implements RandomAccess

To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states #3785

To improve accord interoperability test coverage, need to extend the harry model domain to handle more possible CQL states #3785

Conversation

dcapwell commented Jan 13, 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

aweisberg left a 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

aweisberg left a 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

aweisberg Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcapwell commented Feb 1, 2025

aweisberg Jan 31, 2025 •

edited

Loading