korlov42 commented on code in PR #7725:
URL: https://github.com/apache/ignite-3/pull/7725#discussion_r2910407170
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/CatalogTableTest.java:
##########
@@ -1534,6 +1535,208 @@ private TestColumnTypeParams(ColumnType type, @Nullable
Integer precision, @Null
}
}
+ @Test
+ public void testAddMultipleColumnsAssignsSequentialIds() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ CatalogTableDescriptor tableBefore = actualTable(TABLE_NAME);
+ assertNotNull(tableBefore);
+ int maxId =
tableBefore.columns().stream().mapToInt(CatalogTableColumnDescriptor::id).max().orElse(0);
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParams("COL_A", INT32, true),
+ columnParams("COL_B", STRING, 100, true),
+ columnParams("COL_C", DECIMAL, true, DFLT_TEST_PRECISION, 2)
+ ));
+
+ CatalogTableDescriptor table = actualTable(TABLE_NAME);
+ assertNotNull(table);
+ assertEquals(maxId + 1, table.column("COL_A").id());
+ assertEquals(maxId + 2, table.column("COL_B").id());
+ assertEquals(maxId + 3, table.column("COL_C").id());
+ }
+
+ @Test
+ public void testAddMultipleColumnsPreservesProperties() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParamsBuilder("COL_INT", INT32,
true).defaultValue(constant(42)).build(),
+ columnParamsBuilder("COL_STR",
STRING).length(200).defaultValue(constant("hello")).build(),
+ columnParams("COL_DEC", DECIMAL, true, 15, 5)
+ ));
+
+ CatalogTableDescriptor table = actualTable(TABLE_NAME);
+ assertNotNull(table);
+
+ CatalogTableColumnDescriptor colInt = table.column("COL_INT");
+ assertNotNull(colInt);
+ assertEquals(INT32, colInt.type());
+ assertTrue(colInt.nullable());
+ assertEquals(constant(42), colInt.defaultValue());
+
+ CatalogTableColumnDescriptor colStr = table.column("COL_STR");
+ assertNotNull(colStr);
+ assertEquals(STRING, colStr.type());
+ assertEquals(200, colStr.length());
+ assertEquals(constant("hello"), colStr.defaultValue());
+
+ CatalogTableColumnDescriptor colDec = table.column("COL_DEC");
+ assertNotNull(colDec);
+ assertEquals(DECIMAL, colDec.type());
+ assertTrue(colDec.nullable());
+ assertEquals(15, colDec.precision());
+ assertEquals(5, colDec.scale());
+ }
+
+ @Test
+ public void testAddMultipleColumnsPreservesOrder() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParams("COL_A", INT32, true),
+ columnParams("COL_B", INT32, true),
+ columnParams("COL_C", INT32, true)
+ ));
+
+ CatalogTableDescriptor table = actualTable(TABLE_NAME);
+ assertNotNull(table);
+ List<CatalogTableColumnDescriptor> columns = table.columns();
+
+ // simpleTable creates 6 columns, new ones should be at indices 6, 7, 8
+ assertEquals("COL_A", columns.get(6).name());
+ assertEquals("COL_B", columns.get(7).name());
+ assertEquals("COL_C", columns.get(8).name());
+
+ assertEquals(6, table.columnIndex("COL_A"));
+ assertEquals(7, table.columnIndex("COL_B"));
+ assertEquals(8, table.columnIndex("COL_C"));
+ }
+
+ @Test
+ public void testAddMultipleColumnsIncrementsTableVersionOnce() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ CatalogTableDescriptor tableBefore = actualTable(TABLE_NAME);
+ assertNotNull(tableBefore);
+ assertEquals(1, tableBefore.latestSchemaVersion());
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParams("COL_A", INT32, true),
+ columnParams("COL_B", INT32, true),
+ columnParams("COL_C", INT32, true)
+ ));
+
+ CatalogTableDescriptor tableAfter = actualTable(TABLE_NAME);
+ assertNotNull(tableAfter);
+ assertEquals(2, tableAfter.latestSchemaVersion());
+ }
+
+ @Test
+ public void testAddMultipleColumnsTimeTravelVisibility() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ long timestampBeforeAdd = clock.nowLong();
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParams("COL_A", INT32, true),
+ columnParams("COL_B", INT32, true)
+ ));
+
+ // At the old timestamp, the new columns should not be visible.
+ CatalogTableDescriptor oldTable =
manager.activeCatalog(timestampBeforeAdd).table(SCHEMA_NAME, TABLE_NAME);
+ assertNotNull(oldTable);
+ assertNull(oldTable.column("COL_A"));
+ assertNull(oldTable.column("COL_B"));
+
+ // At the latest timestamp, the new columns should be visible.
+ CatalogTableDescriptor newTable = actualTable(TABLE_NAME);
+ assertNotNull(newTable);
+ assertNotNull(newTable.column("COL_A"));
+ assertNotNull(newTable.column("COL_B"));
+ }
+
+ @Test
+ public void testAddMultipleColumnsFiresEventWithAllColumns() {
+ tryApplyAndExpectApplied(simpleTable(TABLE_NAME));
+
+ var fireEventFuture = new CompletableFuture<Void>();
+
+ manager.listen(CatalogEvent.TABLE_ALTER, fromConsumer(fireEventFuture,
(AddColumnEventParameters parameters) -> {
+ List<CatalogTableColumnDescriptor> descriptors =
parameters.descriptors();
+
+ assertThat(descriptors, hasSize(2));
+ assertEquals("COL_A", descriptors.get(0).name());
+ assertEquals("COL_B", descriptors.get(1).name());
+ }));
+
+ tryApplyAndExpectApplied(addColumnParams(TABLE_NAME,
+ columnParams("COL_A", INT32, true),
+ columnParams("COL_B", INT32, true)
+ ));
+
+ assertThat(fireEventFuture, willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testAddMultipleColumnsIfNotExistsPartialOverlap() {
Review Comment:
Let's add similar test for DROP COLUMN case. Also, it would be nice to have
`testAddMultipleColumnsIfNotExistsAllExist` and
`testAddMultipleColumnsFailsAtomicallyWhenOneAlreadyExists`
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/SqlDdlParserTest.java:
##########
@@ -1169,6 +1169,94 @@ public void alterTableAddColumnNotNull() {
expectUnparsed(addColumn, "ALTER TABLE \"T\" ADD COLUMN \"C\" INTEGER
NOT NULL");
}
+ @Test
+ public void alterTableAddMultipleColumns() {
+ SqlNode sqlNode = parse("ALTER TABLE t ADD COLUMN (a INT, b VARCHAR, c
DECIMAL(10, 2))");
+
+ IgniteSqlAlterTableAddColumn addColumn =
assertInstanceOf(IgniteSqlAlterTableAddColumn.class, sqlNode);
+
+ assertThat(addColumn.name.names, is(List.of("T")));
+ assertThat(addColumn.ifColumnNotExists(), is(false));
+ assertThat(addColumn.columns().size(), is(3));
+
+ SqlColumnDeclaration col0 = (SqlColumnDeclaration)
addColumn.columns().get(0);
+ SqlColumnDeclaration col1 = (SqlColumnDeclaration)
addColumn.columns().get(1);
+ SqlColumnDeclaration col2 = (SqlColumnDeclaration)
addColumn.columns().get(2);
+
+ expectColumnBasic(col0, "A", ColumnStrategy.NULLABLE, "INTEGER", true);
+ expectColumnBasic(col1, "B", ColumnStrategy.NULLABLE, "VARCHAR", true);
+ expectColumnBasic(col2, "C", ColumnStrategy.NULLABLE, "DECIMAL", true);
+
+ expectUnparsed(addColumn, "ALTER TABLE \"T\" ADD COLUMN \"A\" INTEGER,
\"B\" VARCHAR, \"C\" DECIMAL(10, 2)");
+ }
+
+ @Test
+ public void alterTableAddMultipleColumnsIfNotExists() {
+ SqlNode sqlNode = parse("ALTER TABLE t ADD COLUMN IF NOT EXISTS (a
INT, b VARCHAR NOT NULL)");
+
+ IgniteSqlAlterTableAddColumn addColumn =
assertInstanceOf(IgniteSqlAlterTableAddColumn.class, sqlNode);
+
+ assertThat(addColumn.name.names, is(List.of("T")));
+ assertThat(addColumn.ifColumnNotExists(), is(true));
+ assertThat(addColumn.columns().size(), is(2));
+
+ SqlColumnDeclaration col0 = (SqlColumnDeclaration)
addColumn.columns().get(0);
+ SqlColumnDeclaration col1 = (SqlColumnDeclaration)
addColumn.columns().get(1);
+
+ expectColumnBasic(col0, "A", ColumnStrategy.NULLABLE, "INTEGER", true);
+ expectColumnBasic(col1, "B", ColumnStrategy.NOT_NULLABLE, "VARCHAR",
false);
+
+ expectUnparsed(addColumn, "ALTER TABLE \"T\" ADD COLUMN IF NOT EXISTS
\"A\" INTEGER, \"B\" VARCHAR NOT NULL");
+ }
+
+ @Test
+ public void alterTableAddMultipleColumnsWithDefaults() {
+ SqlNode sqlNode = parse("ALTER TABLE t ADD COLUMN (a INT DEFAULT 1, b
VARCHAR DEFAULT 'hello')");
+
+ IgniteSqlAlterTableAddColumn addColumn =
assertInstanceOf(IgniteSqlAlterTableAddColumn.class, sqlNode);
+
+ assertThat(addColumn.columns().size(), is(2));
+
+ SqlColumnDeclaration col0 = (SqlColumnDeclaration)
addColumn.columns().get(0);
+ SqlColumnDeclaration col1 = (SqlColumnDeclaration)
addColumn.columns().get(1);
+
+ assertNotNull(col0.expression);
+ assertNotNull(col1.expression);
+
+ expectUnparsed(addColumn, "ALTER TABLE \"T\" ADD COLUMN \"A\" INTEGER
DEFAULT (1), \"B\" VARCHAR DEFAULT ('hello')");
+ }
+
+ @Test
+ public void alterTableAddMultipleColumnsWithoutColumnKeyword() {
Review Comment:
I would propose to add a few more tests:
- One is similar to this one but with `IF NOT EXISTS` after `ADD` (to make
sure there is no conflict if optional word is omitted)
- Second is in fact group of faulty cases: we need to make sure that
exception refers to a proper position. See `tablePropertiesParsingErrors` for
reference
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
