CASSANDRA-19664 first approach - for copying Basic Table definition #3718

Maxwell-Guo · 2024-12-02T13:10:32Z

This is for CASSANDRA-19664:
1、we can copy table under the same keyspace of under different keysapces.
2、only support basic table schema : primary key(partition key and clustering key), regular columns , static columns, column masking, table params(compaction, compression and so on).

Maxwell-Guo · 2024-12-02T13:16:37Z

src/antlr/Parser.g

@@ -236,6 +236,7 @@ cqlStatement returns [CQLStatement.Raw stmt]
    | st42=addIdentityStatement            { $stmt = st42; }
    | st43=dropIdentityStatement           { $stmt = st43; }
    | st44=listSuperUsersStatement         { $stmt = st44; }
+    | st45=copyTableStatement              { $stmt = st45; }


The reason i used a new statement here is that :
1、The original createtablestatment code is too bloated. If the semantics of like are added, the code needs to be changed a lot.
2、In addition, I think this new copystatement can do things other than table cloning, and can also support semantic capabilities such as “CREATE TABLE AS”. Because they are similar to table copying functions, they are called copystatement here.

Besides, I have also prepared a patch, which is mainly to modify the pares of create table, that is, to implement the function of create table like in create table statement. If you think it is necessary, I can create a new PR to replace the current one.

There is another pr here that implement CASSANDRA-19964, but use create table's parase path .
I am not sure which one is better , so I attach both of them.

Maxwell-Guo · 2024-12-05T13:11:57Z

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

+            throw ire("Cannot use CTREATE TABLE LIKE across different keyspace when source table have UDTs.");
+        }
+
+        String sourceCQLString = sourceTableMeta.toCqlString(false, false, true, false);


we ignore the dropped columns, if the seconds input argument is true that means the dropped column will be consider .
Then the cql will be like : create table tb (pk int primary key, cn int, dn int) with xxxx ; alter table tb drop dn;

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

smiklosovic · 2024-12-05T14:08:13Z

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

+    }
+
+    @Override
+    public void validate(ClientState state)


why do we need this? Will not be validate of super called automatically when you do not override?

yes , we can remove it.

You also need to do this, no?

https://github.com/apache/cassandra/pull/3721/files#r1871469974

@Maxwell-Guo please cover my last comment.

Hi , I replied here, I think we may not need to do this , because the CQLs that are parsed are all create table like syntax, there will be no coexistence with create table syntax. For cql like create table if not exist ta like tb or some other cql that contains these three keywords : CREATE / TABLE / LIKE , only create table a like b can be successfully paresd into copytablestatemnt, others will got a SyntaxException. I have a test for this in CreateLikeCqlParseTest.

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

src/java/org/apache/cassandra/schema/DroppedColumn.java

src/java/org/apache/cassandra/schema/TableMetadata.java

test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java

blerer

This is my first round of review. :-) Thanks a lot for pushing that functionality forward.

src/java/org/apache/cassandra/cql3/QueryProcessor.java

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

test/unit/org/apache/cassandra/cql3/CQLTester.java

blerer · 2024-12-06T16:55:07Z

test/unit/org/apache/cassandra/cql3/statements/DescribeStatementTest.java

+        String souceTable = createTable(KEYSPACE_PER_TEST,
+                                  "CREATE TABLE %s (" +
+                                        "  pk1 text, " +
+                                        "  pk2 int MASKED WITH DEFAULT, " +
+                                        "  ck1 int, " +
+                                        "  ck2 double," +
+                                        "  s1 float static, " +
+                                        "  v1 int, " +
+                                        "  v2 int, " +
+                                        "PRIMARY KEY ((pk1, pk2), ck1, ck2 ))");
+        String targetTable = createTableLike("create table %s like %s", souceTable, KEYSPACE_PER_TEST, KEYSPACE_PER_TEST);
+        Pair<TableMetadata, TableMetadata> pair = assertTableMetaEquals(KEYSPACE_PER_TEST, KEYSPACE_PER_TEST, souceTable, targetTable);


It could be replaced by:

String souceTable = createTable(KEYSPACE_PER_TEST, "CREATE TABLE %s (" + " pk1 text, " + " pk2 int MASKED WITH DEFAULT, " + " ck1 int, " + " ck2 double," + " s1 float static, " + " v1 int, " + " v2 int, " + "PRIMARY KEY ((pk1, pk2), ck1, ck2 ))"); TableMetadata sourceTableMetadata = currentTableMetadata(); String targetTable = createTableLike("create table %s like %s", souceTable, KEYSPACE_PER_TEST, KEYSPACE_PER_TEST); TableMetadata targetTableMetadata = currentTableMetadata(); assertTableMetaEquals(sourceTableMetadata, targetTableMetadata);

and assertTableMetaEquals can be moved to DescribeStatementTest and replaced by:

protected void assertTableMetaEquals(TableMetadata source, TableMetadata target) { assertNotNull(source); assertNotNull(target); assertTrue(source.equalsWithoutTableNameAndDropCns(target)); assertNotEquals(source.id, target.id); assertNotEquals(source.name, target.name); }

It will then allow to remove the getTableMetadata methods in CQLTester.

assertTableMetaEquals will aslo be used for other test classes like
CreateLikeTest -> testTableShemaCopy ()

I remove the getTableMetadata in CQLTester

I change the test code in DescribeStatementTest and remove the use of assertTableMetaEquals, and verify them one by one :

assertNotNull(source); String targetTable = createTableLike("create table %s like %s", souceTable, KEYSPACE_PER_TEST, KEYSPACE_PER_TEST); TableMetadata target = getTableMetadata(KEYSPACE_PER_TEST, currentTable()); assertNotNull(target); assertTrue(equalsWithoutTableNameAndDropCns(source, target)); assertNotEquals(source.id, target.id); assertNotEquals(source.name, target.name);

That is because
(1) currentTableMetadata(); only return the table meta under KEYSPACE;
(2) the assertTableMetaEquals is only used once, but it is used many time in CreateLikeTest so I move assertTableMetaEquals to CreateLikeTest,

WDYT ? @blerer

blerer · 2024-12-06T16:57:08Z

test/unit/org/apache/cassandra/cql3/CQLTester.java

+    protected TableMetadata getTableMetadata(String table)
+    {
+        return Schema.instance.getTableMetadata(KEYSPACE, table);
+    }
+
+    protected TableMetadata getTableMetadata(String keyspace, String table)
+    {
+        return Schema.instance.getTableMetadata(keyspace, table);
+    }
+


Can be removed after the changes suggested in DescribeStatementTest

see the comment on assertTableMetaEquals

@blerer
I removed

{ return Schema.instance.getTableMetadata(KEYSPACE, table); }

But kept

protected TableMetadata getTableMetadata(String keyspace, String table) { return Schema.instance.getTableMetadata(keyspace, table); }

as getTableMetadata(String keyspace, String table) is used by some test cases, is it ok ?

blerer · 2024-12-06T17:06:51Z

src/java/org/apache/cassandra/cql3/statements/schema/CreateTableStatement.java

@@ -479,12 +479,21 @@ public String defaultCompactValueName()
        }
    }

+    public static TableMetadata.Builder parse(String cql, String keyspace, String table, Types types, UserFunctions userFunctions)


I feel that I am missing something. Why did you change that code?

@blerer the original version of that method (which calls this one) is still there. We just create this one in order to be able to specify custom table name with types and functions. If we parse CQL for old table, we just want to be able to put there table name etc for new table directly upon parsing.

Maxwell-Guo · 2024-12-09T12:07:21Z

test/unit/org/apache/cassandra/cql3/CQLTester.java

+    protected TableMetadata getTableMetadata(String table)
+    {
+        return Schema.instance.getTableMetadata(KEYSPACE, table);
+    }
+
+    protected TableMetadata getTableMetadata(String keyspace, String table)
+    {
+        return Schema.instance.getTableMetadata(keyspace, table);
+    }
+


@blerer
I removed

{ return Schema.instance.getTableMetadata(KEYSPACE, table); }

But kept

protected TableMetadata getTableMetadata(String keyspace, String table) { return Schema.instance.getTableMetadata(keyspace, table); }

as getTableMetadata(String keyspace, String table) is used by some test cases, is it ok ?

Maxwell-Guo · 2024-12-09T12:19:45Z

test/unit/org/apache/cassandra/schema/createlike/CreateLikeTest.java

+    @Test
+    public void testIfNotExists() throws Throwable
+    {
+        String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int, b text, c duration, d float, PRIMARY KEY(a, b));");


@blerer I add one test case for the IF NOT EXISTS as you suggested

Maxwell-Guo · 2024-12-09T12:21:11Z

test/unit/org/apache/cassandra/schema/createlike/CreateLikeTest.java

+        assertTableMetaEquals(sourceKs, targetKs, tableOtherOptions, tbLikeCompactionOthers);
+
+        // table copy with params setting
+        String tableCopyAndSetCompression = createTableLike("CREATE TABLE %s LIKE %s WITH compression = { 'class' : 'SnappyCompressor', 'chunk_length_in_kb' : 64 };",


@smiklosovic I update the pr and implemented setting table params when doing CREARE TABLE LIKE

Maxwell-Guo · 2024-12-09T12:23:07Z

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

+import org.apache.cassandra.transport.Event.SchemaChange;
+
+/**
+ * {@code CREATE TABLE [IF NOT EXISTS] <newtable> LIKE <oldtable> WITH <property> = <value>}


@blerer this is a brief introduction of cql to CopyTableStatement, and I have already create CASSANDRA-20120 to create CEP-43's doc

smiklosovic · 2024-12-11T10:23:16Z

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

+                                           .build();
+        table.validate();
+
+        if (targetKeyspaceMeta.replicationStrategy.hasTransientReplicas()


@Maxwell-Guo This is interesting corner-case. I think that if there was not a possibility to specify different source keyspace from the target one, then we would not need this ... Hm. I think it needs to be there then.

My idea at the time was that copying a table is also an act of creating a new table, the only difference is that target table has same schema with source table.

We have no way to control the target keyspace because the keyspaces of the two tables may be the same or completely different.

So I kept it.

smiklosovic · 2024-12-11T10:29:10Z

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java

+            throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
+        }
+
+        if (!table.params.compression.isEnabled())


@Maxwell-Guo

So this says that: "if the compression is not enabled on the source table, then be sure that guardrail enables uncompressed sstables".

While it does make sense when creating the original table for the first time, does it still make sense to do when we are copying it?

If we disabled the compression on source table, then I think that we already do have a guardrail for that, correct? But if we change the guardrail in runtime after the original table was created (e.g. via JMX), then copying such table will not be possible anymore?

So, whether we can copy a table or not when the source table has disabled compression only depends on what the guardrail is set to in the runtime.

I prefer to not check this. I would rather create a copy of the source table no matter what and we can propagate back to the client (the message will appear in cqlsh when copying table) that if guardrail is different, then a user might notified about that, but this is optional.

If we disabled the compression on source table, then I think that we already do have a guardrail for that, correct? But if we change the guardrail in runtime after the original table was created (e.g. via JMX), then copying such table will not be possible anymore?

Yes, that is why I kept it.

The interval between creating two tables may be long, and it is uncertain whether the user has any new ideas about table compression during this period. Therefore, for the behavior of copying the table, except for copying the table schema(column, datatype, mask, table params), creating udt, index, trigger (if needed), and do some validations on udt、indexes and triggers, I have aligned other behaviors and validations with creating a new table.

Thinking about it again, I think how it is done is OK, because we can also do create table ks.tb_copy like ks.tb with compression = disabled and using the guardrail here is perfectly sensible.

Maxwell-Guo commented Dec 2, 2024

View reviewed changes

Maxwell-Guo force-pushed the CASSANDRA-19964 branch 3 times, most recently from 52f4d30 to 8b09b40 Compare December 3, 2024 12:07

CREATE TABLE LIKE for copying Basic Table definition

b826a62

Maxwell-Guo force-pushed the CASSANDRA-19964 branch from 8b09b40 to b826a62 Compare December 3, 2024 13:53

smiklosovic changed the title ~~CREATE TABLE LIKE for copying Basic Table definition for CASSANDRA-19664~~ CASSANDRA-19664 first approach - for copying Basic Table definition Dec 3, 2024

modification for review

ec4fd9a

Maxwell-Guo commented Dec 5, 2024

View reviewed changes

smiklosovic reviewed Dec 5, 2024

View reviewed changes

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java Outdated Show resolved Hide resolved

smiklosovic reviewed Dec 5, 2024

View reviewed changes

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java Outdated Show resolved Hide resolved

smiklosovic reviewed Dec 5, 2024

View reviewed changes

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java Outdated Show resolved Hide resolved

smiklosovic reviewed Dec 5, 2024

View reviewed changes

src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java Outdated Show resolved Hide resolved

remove some braces, toString and validate

e40726c

Maxwell-Guo force-pushed the CASSANDRA-19964 branch from 173c775 to e40726c Compare December 6, 2024 08:51

smiklosovic reviewed Dec 6, 2024

View reviewed changes

src/java/org/apache/cassandra/schema/DroppedColumn.java Outdated Show resolved Hide resolved

smiklosovic reviewed Dec 6, 2024

View reviewed changes

src/java/org/apache/cassandra/schema/TableMetadata.java Outdated Show resolved Hide resolved

smiklosovic reviewed Dec 6, 2024

View reviewed changes

test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java Outdated Show resolved Hide resolved

blerer reviewed Dec 6, 2024

View reviewed changes

Maxwell-Guo force-pushed the CASSANDRA-19964 branch 2 times, most recently from 0a7b1f5 to ed700a8 Compare December 9, 2024 12:13

add table params to create table like and fix some review comments

bbe3ad6

Maxwell-Guo force-pushed the CASSANDRA-19964 branch from ed700a8 to bbe3ad6 Compare December 9, 2024 12:16

Maxwell-Guo commented Dec 9, 2024

View reviewed changes

smiklosovic reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSANDRA-19664 first approach - for copying Basic Table definition #3718

CASSANDRA-19664 first approach - for copying Basic Table definition #3718

Maxwell-Guo commented Dec 2, 2024

Maxwell-Guo Dec 2, 2024

Maxwell-Guo Dec 3, 2024

Maxwell-Guo Dec 5, 2024

smiklosovic Dec 5, 2024

Maxwell-Guo Dec 5, 2024

smiklosovic Dec 5, 2024 •

edited

Loading

smiklosovic Dec 5, 2024

Maxwell-Guo Dec 5, 2024

blerer left a comment

blerer Dec 6, 2024

Maxwell-Guo Dec 9, 2024 •

edited

Loading

blerer Dec 6, 2024

Maxwell-Guo Dec 9, 2024

Maxwell-Guo Dec 9, 2024 •

edited

Loading

blerer Dec 6, 2024

smiklosovic Dec 8, 2024 •

edited

Loading

Maxwell-Guo Dec 9, 2024 •

edited

Loading

Maxwell-Guo Dec 9, 2024

Maxwell-Guo Dec 9, 2024

Maxwell-Guo Dec 9, 2024

smiklosovic Dec 11, 2024 •

edited

Loading

Maxwell-Guo Dec 11, 2024 •

edited

Loading

smiklosovic Dec 12, 2024

smiklosovic Dec 11, 2024 •

edited

Loading

Maxwell-Guo Dec 11, 2024

smiklosovic Dec 12, 2024 •

edited

Loading

CASSANDRA-19664 first approach - for copying Basic Table definition #3718

Are you sure you want to change the base?

CASSANDRA-19664 first approach - for copying Basic Table definition #3718

Conversation

Maxwell-Guo commented Dec 2, 2024

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

smiklosovic Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blerer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maxwell-Guo Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maxwell-Guo Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smiklosovic Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

Maxwell-Guo Dec 9, 2024 • 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

smiklosovic Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Maxwell-Guo Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smiklosovic Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smiklosovic Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

smiklosovic Dec 5, 2024 •

edited

Loading

Maxwell-Guo Dec 9, 2024 •

edited

Loading

Maxwell-Guo Dec 9, 2024 •

edited

Loading

smiklosovic Dec 8, 2024 •

edited

Loading

Maxwell-Guo Dec 9, 2024 •

edited

Loading

smiklosovic Dec 11, 2024 •

edited

Loading

Maxwell-Guo Dec 11, 2024 •

edited

Loading

smiklosovic Dec 11, 2024 •

edited

Loading

smiklosovic Dec 12, 2024 •

edited

Loading