bbotella commented on code in PR #3831:
URL: https://github.com/apache/cassandra/pull/3831#discussion_r1930760576
##########
pylib/cqlshlib/cql3handling.py:
##########
@@ -380,6 +380,11 @@ def dequote_value(cqlword):
( ender="," [propmapkey]=<term> ":"
[propmapval]=<term> )*
ender="}"
;
+<propertyOrOption> ::= <property>
+ | "ALL"
+ | "INDEX"
Review Comment:
Reading the CEP, I think these names should be plural right? `INDEXES` and
`TRIGGERS`?
##########
test/unit/org/apache/cassandra/cql3/CQLTester.java:
##########
@@ -1969,24 +1981,82 @@ private static boolean isEmptyContainerNull(DataType
type,
}
/**
- * Determine whether the source and target TableMetadata is equal without
compare the table name and dropped columns.
+ * Determine whether the source and target TableMetadata is equal without
compare the table name and dropped columns, and
+ * compare the indexes and triggers of the source and target tables, but
do not compare their names.
* @param source the source TableMetadata
* @param target the target TableMetadata
- * @param compareParams wether compare table params
+ * @param compareParams wether compare table's params
* @param compareIndexes wether compare table's indexes
- * @param compareTrigger wether compare table's triggers
+ * @param compareTriggers wether compare table's triggers
* */
- protected boolean equalsWithoutTableNameAndDropCns(TableMetadata source,
TableMetadata target, boolean compareParams, boolean compareIndexes, boolean
compareTrigger)
+ protected boolean equalsWithoutTableNameAndDropCns(TableMetadata source,
TableMetadata target, boolean compareParams, boolean compareIndexes, boolean
compareTriggers)
{
return source.partitioner.equals(target.partitioner)
&& source.kind == target.kind
&& source.flags.equals(target.flags)
&& (!compareParams || source.params.equals(target.params))
- && (!compareIndexes || source.indexes.equals(target.indexes))
- && (!compareTrigger || source.triggers.equals(target.triggers))
+ && compareIndexesOrTriggerWithoutName(compareIndexes,
+ compareTriggers,
+ source.indexes,
+ target.indexes,
+ source.triggers,
+ target.triggers,
+ target.keyspace,
+ target.name)
&& columnsEqualWitoutKsTb(source, target);
}
+ /**
+ * Compare the source table and target table's indexes and triggers
without compare their name,
+ * because the names of the target indexes and triggers are randomly
genrated base on the source
+ * table name and source keyspace name with a rule thant can see
+ * {@link KeyspaceMetadata#findAvailableTriggerName(String)} and {@link
KeyspaceMetadata#findAvailableIndexName(String)}
+ * for more detail.
+ * @param compareIndexes wether compare indexes or not
+ * @param compareTriggers wether compare triggers or not
+ * @param sourceIdxes source indexes
+ * @param targetIdxes target indexes
+ * @param sourceTriggers source triggers
+ * @param targetTriggers target triggers
+ * @return true if source table's indexes or triggers are same with target
table's indexes or triggers without compare keyspace and table name
+ * */
+ private boolean compareIndexesOrTriggerWithoutName(boolean compareIndexes,
boolean compareTriggers, Indexes sourceIdxes, Indexes targetIdxes, Triggers
sourceTriggers, Triggers targetTriggers, String targetKs, String targetTb)
Review Comment:
Similar nit. I think splitting this method into indexes and triggers may
help with the method signature.
##########
src/java/org/apache/cassandra/cql3/statements/schema/CopyTableStatement.java:
##########
@@ -227,13 +237,50 @@ public void validate(ClientState state)
Guardrails.tables.guard(totalUserTables + 1, targetTableName,
false, state);
}
validateDefaultTimeToLive(attrs.asNewTableParams());
+
+ if (createLikeOptions.contains(ALL) && createLikeOptions.size() > 1)
+ {
+ throw ire("It is not allowed to use ALL option together with " +
createLikeOptions.stream().filter(option -> option !=
ALL).sorted().collect(Collectors.toList()));
+ }
+ }
+
+ private void cloneIndexesAndTriggersIfNeeded(TableMetadata.Builder
builder, TableMetadata sourceTableMeta, KeyspaceMetadata targetKeyspaceMeta,
String targetTableName)
+ {
+ if (createLikeOptions.contains(INDEX) ||
+ createLikeOptions.contains(ALL))
+ {
+ Indexes.Builder idxBuilder = Indexes.builder();
+ for (IndexMetadata indexMetadata : sourceTableMeta.indexes)
+ {
+ // todo use source index name if target keyspace don't have
same index name but use random name now as cep-43 described
+ String baseName = targetKeyspaceMeta.name + "_" +
targetTableName + "_" + indexMetadata.name;
+ String idxName =
targetKeyspaceMeta.findAvailableIndexName(baseName);
+ idxBuilder.add(IndexMetadata.fromSchemaMetadata(idxName,
indexMetadata.kind, indexMetadata.options));
+ }
+ builder.indexes(idxBuilder.build());
Review Comment:
Nit: I think extracting this and the triggers section to their own private
methods may help with readibility.
##########
test/unit/org/apache/cassandra/schema/createlike/CreateLikeTest.java:
##########
@@ -576,28 +581,135 @@ public void testTriggerOperationOnCopiedTable()
public void testUnSupportedSchema() throws Throwable
{
createTable(sourceKs, "CREATE TABLE %s (a int PRIMARY KEY, b int, c
text)", "tb");
- String index = createIndex( "CREATE INDEX ON " + sourceKs + ".tb (c)");
+ String index = createIndex("CREATE INDEX ON " + sourceKs + ".tb (c)");
assertInvalidThrowMessage("Souce Table '" + targetKs + "." + index +
"' doesn't exist", InvalidRequestException.class,
- "CREATE TABLE " + sourceKs + ".newtb LIKE " +
targetKs + "." + index + ";");
+ "CREATE TABLE " + sourceKs + ".newtb LIKE "
+ targetKs + "." + index + ";");
assertInvalidThrowMessage("System keyspace 'system' is not
user-modifiable", InvalidRequestException.class,
"CREATE TABLE system.local_clone LIKE
system.local ;");
assertInvalidThrowMessage("System keyspace 'system_views' is not
user-modifiable", InvalidRequestException.class,
"CREATE TABLE system_views.newtb LIKE
system_views.snapshots ;");
}
+ @Test
+ public void testTableCopyWithIndex()
+ {
+ String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int
PRIMARY KEY, b int, c text, d int)", "sourcetb");
+ createIndex(sourceKs, "CREATE INDEX ON %s (b)");
+ createIndex(sourceKs, "CREATE INDEX ON %s (c)");
+ String targetTb = createTableLike("CREATE TABLE %s LIKE %s WITH INDEX
", sourceTb, sourceKs, targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb, targetTb);
+ }
+
+ @Test
+ public void testTableCopyWithTrigger()
+ {
+ String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int
PRIMARY KEY, b int, c text, d int)", "sourcetb");
+ createTrigger(sourceKs, "sourcetb", "trigger_1", "CREATE TRIGGER %s ON
%s USING '" + triggerClass1 + "'");
+ createTrigger(sourceKs, "sourcetb", "trigger_2", "CREATE TRIGGER %s ON
%s USING '" + triggerClass2 + "'");
+ String targetTb = createTableLike("CREATE TABLE %s LIKE %s WITH
TRIGGER", sourceTb, sourceKs, targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb, targetTb);
+ }
+
+ @Test
+ public void testTableCopyWithIndexAndTrigger()
+ {
+ String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int
PRIMARY KEY, b int, c text, d int)", "sourcetb");
+ createIndex(sourceKs, "CREATE INDEX ON %s (b)");
+ createIndex(sourceKs, "CREATE INDEX ON %s (d)");
+ createTrigger(sourceKs, "sourcetb", "trigger_1", "CREATE TRIGGER %s ON
%s USING '" + triggerClass1 + "'");
+ createTrigger(sourceKs, "sourcetb", "trigger_2", "CREATE TRIGGER %s ON
%s USING '" + triggerClass2 + "'");
+ createTrigger(sourceKs, "sourcetb", "trigger_3", "CREATE TRIGGER %s ON
%s USING '" + triggerClass2 + "'");
+ String targetTb = createTableLike("CREATE TABLE %s LIKE %s WITH
TRIGGER AND INDEX", sourceTb, sourceKs, targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb, targetTb);
+ }
+
+ @Test
+ public void testTableCopyWithAllOption()
+ {
+ String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int
PRIMARY KEY, b int, c text, d int)", "sourcetb");
+ createIndex(sourceKs, "CREATE INDEX ON %s (b)");
+ createTrigger(sourceKs, "sourcetb", "trigger_1", "CREATE TRIGGER %s ON
%s USING '" + triggerClass1 + "'");
+ createTrigger(sourceKs, "sourcetb", "trigger_2", "CREATE TRIGGER %s ON
%s USING '" + triggerClass2 + "'");
+ String targetTb = createTableLike("CREATE TABLE %s LIKE %s WITH ALL ",
sourceTb, sourceKs, targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb, targetTb);
+ }
+
+ @Test
+ public void testTableCopyOptionAndTableProperty()
+ {
+ String sourceTb = createTable(sourceKs, "CREATE TABLE %s (a int
PRIMARY KEY, b int, c text, d int)", "sourcetb");
+ createIndex(sourceKs, "CREATE INDEX ON %s (b)");
+ createTrigger(sourceKs, "sourcetb", "trigger_1", "CREATE TRIGGER %s ON
%s USING '" + triggerClass1 + "'");
+ String targetTbWithAll = createTableLike("CREATE TABLE %s LIKE %s WITH
ALL AND crc_check_chance = 0.8 AND cdc = true", sourceTb, sourceKs, targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb,
targetTbWithAll, false, false, false);
+
+ TableMetadata source = getTableMetadata(sourceKs, sourceTb);
+ TableMetadata target = getTableMetadata(targetKs, targetTbWithAll);
+ assertNotEquals(source.params, target.params);
+ assertTrue(target.params.cdc);
+ assertFalse(source.params.cdc);
+ assertEquals(0.8, target.params.crcCheckChance, 0.0);
+
+ String targetTbWithTrigger = createTableLike("CREATE TABLE %s LIKE %s
WITH TRIGGER AND crc_check_chance = 0.8 AND cdc = true", sourceTb, sourceKs,
targetKs);
+ assertTableMetaEqualsWithoutKs(sourceKs, targetKs, sourceTb,
targetTbWithTrigger, false, false, false);
+
+ source = getTableMetadata(sourceKs, sourceTb);
+ target = getTableMetadata(targetKs, targetTbWithTrigger);
+ assertNotEquals(source.params, target.params);
+ assertTrue(target.params.cdc);
+ assertFalse(source.params.cdc);
+ assertEquals(0.8, target.params.crcCheckChance, 0.0);
+ }
+
+ @Test
+ public void testUnSupportedCopyofIndexAndTrigger() throws Throwable
+ {
+ createTable(sourceKs, "CREATE TABLE %s (a int PRIMARY KEY, b int, c
text, d int)", "sourcetb");
+ createIndex(sourceKs, "CREATE INDEX ON %s (b)");
+ createTrigger(sourceKs, "sourcetb", "trigger_1", "CREATE TRIGGER %s ON
%s USING '" + triggerClass1 + "'");
+ // ALL and TRIGGERS is not allowed
+ assertInvalidThrowMessage("It is not allowed to use ALL option
together with [TRIGGER]",
+ InvalidRequestException.class,
+ "CREATE TABLE " + targetKs + ".targettb LIKE
" + sourceKs + ".sourcetb WITH TRIGGER AND ALL");
+ // ALL and INDEXES is not allowed
+ assertInvalidThrowMessage("It is not allowed to use ALL option
together with [INDEX]",
+ InvalidRequestException.class,
+ "CREATE TABLE " + targetKs + ".targettb LIKE
" + sourceKs + ".sourcetb WITH INDEX AND ALL");
+ // ALL and INDEXES and TRIGGERS is not allowed
+ assertInvalidThrowMessage("It is not allowed to use ALL option
together with [INDEX, TRIGGER]",
+ InvalidRequestException.class,
+ "CREATE TABLE " + targetKs + ".targettb LIKE
" + sourceKs + ".sourcetb WITH INDEX AND TRIGGER AND ALL");
+ }
+
Review Comment:
What about, for completion, adding tests for tables without indexes and
triggers that are being copied with `LIKE foo WITH INDEX/TRIGGER/ALL`?
--
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-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
