alex-plekhanov commented on code in PR #13037:
URL: https://github.com/apache/ignite/pull/13037#discussion_r3116289082
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java:
##########
@@ -347,6 +348,208 @@ public void createTableIfNotExists() {
sql("create table if not exists my_table (id int, val varchar)");
}
+ /** Test that it's impossible to create tables with same name regardless
of key/value wrapping settings. */
+ @Test
+ public void createTableWithWrappedKeyVal() {
+ {
+ sql("create table t1 (id int primary key) WITH
\"wrap_value=false\"");
+ sql("create table t2 (id1 int, id2 int, primary key(id1, id2))
WITH \"wrap_value=false\"");
+
+ sql("DROP TABLE t1; DROP TABLE t2");
+ }
+ {
+ sql("create table my_table (id int, val varchar)");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_KEY, by default, this flag is set to false
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_key=true\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_value=false\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_key=true, wrap_value=false\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+ }
+
+ /** Tests wrap=false is forbidden when key or value has more than one
column. */
+ @Test
+ public void testWrappingAlwaysOnWithComplexKV() {
+ assertThrowsSqlException(() -> sql("create table a (id int, x varchar,
c bigint, primary key(id, c)) with \"wrap_key=false\""),
+ "WRAP_KEY parameter cannot be \"false\" when composite primary key
exists.");
+
+ assertThrowsSqlException(() ->
+ sql("create table a (id int, x varchar, c bigint, primary
key(id)) with \"wrap_key=false, key_type=custom\""),
+ "WRAP_KEY parameter cannot be \"false\" when KEY_TYPE is
defined.");
+
+ assertThrowsSqlException(() -> sql("create table a (id int, x varchar,
c bigint, primary key(id)) with \"wrap_value=false\""),
+ "WRAP_VALUE parameter cannot be \"false\" with multiple columns.");
+
+ assertThrowsSqlException(() ->
+ sql("create table a (id int, x varchar, primary key(id)) with
\"wrap_value=false, value_type=custom\""),
+ "WRAP_VALUE parameter cannot be \"false\" when VALUE_TYPE is
defined.");
+ }
+
+ /** */
+ @SuppressWarnings("ThrowableNotThrown")
+ private static void assertThrowsSqlException(Callable<?> call, @Nullable
String msg) {
Review Comment:
Than you need to reuse `assertThrows` with predefined `IgniteSqlException`.
We don't need closure in this method, you never use it other way than `() ->
sql(...)`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -386,6 +423,22 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) &&
cmd.primaryKeyColumns().size() =
res = new QueryEntityEx(res).implicitPk(true);
}
+ if (!cmd.wrapValue()) {
Review Comment:
No, key type should be generated when wrapKey is true, see H2 related code.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/jdbc/JdbcQueryTest.java:
##########
@@ -477,6 +479,186 @@ public void testParametersMetadata() throws Exception {
}
}
+ /**
+ * Test behavior when neither key nor value should be wrapped.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testNoWrap() throws SQLException {
+ doTestKeyValueWrap(false, false, false);
+ }
+
+ /**
+ * Test behavior when only key is wrapped.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testKeyWrap() throws SQLException {
+ doTestKeyValueWrap(true, false, false);
+ }
+
+ /**
+ * Test behavior when only value is wrapped.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testValueWrap() throws SQLException {
+ doTestKeyValueWrap(false, true, false);
+ }
+
+ /**
+ * Test behavior when both key and value is wrapped.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testKeyAndValueWrap() throws SQLException {
+ doTestKeyValueWrap(true, true, false);
+ }
+
+ /**
+ * Test behavior when neither key nor value should be wrapped.
+ * Key and value are UUID.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testUuidNoWrap() throws SQLException {
+ doTestKeyValueWrap(false, false, true);
+ }
+
+ /**
+ * Test behavior when only key is wrapped.
+ * Key and value are UUID.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testUuidKeyWrap() throws SQLException {
+ doTestKeyValueWrap(true, false, true);
+ }
+
+ /**
+ * Test behavior when only value is wrapped.
+ * Key and value are UUID.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testUuidValueWrap() throws SQLException {
+ doTestKeyValueWrap(false, true, true);
+ }
+
+ /**
+ * Test behavior when both key and value is wrapped.
+ * Key and value are UUID.
+ * @throws SQLException if failed.
+ */
+ @Test
+ public void testUuidKeyAndValueWrap() throws SQLException {
+ doTestKeyValueWrap(true, true, true);
+ }
+
+ /**
+ * Test behavior for given combination of wrap flags.
+ * @param wrapKey Whether key wrap should be enforced.
+ * @param wrapVal Whether value wrap should be enforced.
+ * @throws SQLException if failed.
+ */
+ private void doTestKeyValueWrap(boolean wrapKey, boolean wrapVal, boolean
testUuid) throws SQLException {
+ try {
+ String sql = testUuid ? String.format("CREATE TABLE T (\"Id\" UUID
primary key, \"xX\" UUID) WITH " +
+ "\"wrap_key=%b,wrap_value=%b", wrapKey, wrapVal) :
+ String.format("CREATE TABLE T (\"Id\" int primary key, \"xX\"
varchar) WITH " +
+ "\"wrap_key=%b,wrap_value=%b", wrapKey, wrapVal);
+
+ UUID guid = UUID.randomUUID();
+
+ if (wrapKey)
Review Comment:
You still provide "key_type=" in WITH clause, that's why it works. Try
without key_type and compare results with H2.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -212,6 +212,8 @@ private void checkKVWrappedParam(CreateTableCommand cmd) {
IgniteQueryErrorCode.PARSING);
}
}
+ else
+ cmd.wrapValue(true); // By default, value is always wrapped to
allow for ALTER TABLE ADD COLUMN commands.
Review Comment:
Looks like setting to true is redundant, since we check only `if
(cmd.wrapValue() != null && !cmd.wrapValue())`
Let's just move comment near to this check..
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java:
##########
@@ -347,6 +348,208 @@ public void createTableIfNotExists() {
sql("create table if not exists my_table (id int, val varchar)");
}
+ /** Test that it's impossible to create tables with same name regardless
of key/value wrapping settings. */
+ @Test
+ public void createTableWithWrappedKeyVal() {
+ {
+ sql("create table t1 (id int primary key) WITH
\"wrap_value=false\"");
+ sql("create table t2 (id1 int, id2 int, primary key(id1, id2))
WITH \"wrap_value=false\"");
+
+ sql("DROP TABLE t1; DROP TABLE t2");
+ }
+ {
+ sql("create table my_table (id int, val varchar)");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_KEY, by default, this flag is set to false
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_key=true\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_value=false\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true, wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+
+ {
+ sql("create table my_table (id int, val varchar) WITH
\"wrap_key=true, wrap_value=false\"");
+
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar)"),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_key=true\""),
+ "Table already exists: MY_TABLE");
+
+ // WRAP_VALUE, by default, this flag is set to true
+ assertThrowsSqlException(() -> sql("create table my_table (id int,
val varchar) WITH \"wrap_value=false\""),
+ "Table already exists: MY_TABLE");
+
+ sql("DROP TABLE my_table");
+ }
+ }
+
+ /** Tests wrap=false is forbidden when key or value has more than one
column. */
+ @Test
+ public void testWrappingAlwaysOnWithComplexKV() {
+ assertThrowsSqlException(() -> sql("create table a (id int, x varchar,
c bigint, primary key(id, c)) with \"wrap_key=false\""),
+ "WRAP_KEY parameter cannot be \"false\" when composite primary key
exists.");
+
+ assertThrowsSqlException(() ->
+ sql("create table a (id int, x varchar, c bigint, primary
key(id)) with \"wrap_key=false, key_type=custom\""),
+ "WRAP_KEY parameter cannot be \"false\" when KEY_TYPE is
defined.");
+
+ assertThrowsSqlException(() -> sql("create table a (id int, x varchar,
c bigint, primary key(id)) with \"wrap_value=false\""),
+ "WRAP_VALUE parameter cannot be \"false\" with multiple columns.");
+
+ assertThrowsSqlException(() ->
+ sql("create table a (id int, x varchar, primary key(id)) with
\"wrap_value=false, value_type=custom\""),
+ "WRAP_VALUE parameter cannot be \"false\" when VALUE_TYPE is
defined.");
+ }
+
+ /** */
+ @SuppressWarnings("ThrowableNotThrown")
+ private static void assertThrowsSqlException(Callable<?> call, @Nullable
String msg) {
+ GridTestUtils.assertThrows(log, call, IgniteSQLException.class, msg);
+ }
+
+ /**
+ * Test that {@code ADD COLUMN} fails.
+ */
+ @Test
+ public void testAlterTableFailOnSingleCacheValue() {
+ CacheConfiguration c =
+ new CacheConfiguration("ints").setIndexedTypes(Integer.class,
Integer.class)
+ .setSqlSchema(QueryUtils.DFLT_SCHEMA);
+
+ try {
+ client.getOrCreateCache(c);
+
+ doTestAlterTableOnFlatValue("INTEGER");
+ }
+ finally {
+ client.destroyCache("ints");
+ }
+ }
+
+ /**
+ * Test that {@code ADD COLUMN} fails.
+ */
+ @Test
+ public void testAlterTableFailOnSingleTableValue() {
+ try {
+ sql("CREATE TABLE TEST (id INT PRIMARY KEY, x VARCHAR) with
\"wrap_value=false\"");
+
+ doTestAlterTableOnFlatValue("TEST");
+ }
+ finally {
+ sql("DROP TABLE TEST");
+ }
+ }
+
+ /**
+ * Test that {@code ADD COLUMN} fails for tables that have single value.
+ *
+ * @param tblName table name.
+ */
+ private void doTestAlterTableOnFlatValue(String tblName) {
+ assertThrows("ALTER TABLE " + tblName + " ADD COLUMN y varchar",
IgniteSQLException.class,
+ "Cannot add column(s) because table was created");
+ }
+
+ /**
+ * Test single column PK without wrapping calculate correct inline size.
+ */
+ @Test
+ public void testInlineSizeNoWrap() {
+ try {
+ sql("CREATE TABLE IF NOT EXISTS T ( " +
+ " id varchar(15), " +
+ " col varchar(100), " +
+ " PRIMARY KEY(id) ) ");
+ assertEquals(18, sql(
+ "select INLINE_SIZE from SYS.INDEXES where TABLE_NAME = 'T'
and IS_PK = true").get(0).get(0));
+ }
+ finally {
+ sql("DROP TABLE IF EXISTS T");
+ }
+ }
+
+ /**
+ * Test single column PK with wrapping calculate correct inline size.
+ */
+ @Test
+ public void testInlineSizeWrap() {
+ try {
+ sql("CREATE TABLE IF NOT EXISTS T ( " +
+ " id varchar(15), " +
+ " col varchar(100), " +
+ " PRIMARY KEY(id) ) WITH \"wrap_key=true\"");
+ assertEquals(18, sql(
+ "select INLINE_SIZE from SYS.INDEXES where TABLE_NAME = 'T'
and IS_PK = true").get(0).get(0));
+ }
+ finally {
+ sql("DROP TABLE IF EXISTS T");
+ }
+ }
+
+ /**
+ * Test two column PK with wrapping calculate correct inline size.
+ */
+ @Test
+ public void testInlineSizeWrapMultiPk() {
+ try {
+ sql("CREATE TABLE IF NOT EXISTS T ( " +
+ " id varchar(15), " +
+ " id2 uuid, " +
+ " col varchar(100), " +
+ " PRIMARY KEY(id, id2) ) WITH \"wrap_key=true\"");
+ assertEquals(35, sql(
+ "select INLINE_SIZE from SYS.INDEXES where TABLE_NAME = 'T'
and IS_PK = true").get(0).get(0));
+ }
+ finally {
+ sql("DROP TABLE IF EXISTS T");
+ }
+ }
Review Comment:
But `wrap_key=true` in WITH clause and this confusing. Someone may think
that inline size have such a value because wrap_key is true. This test maybe
helpful, but we need to check with wrap_key=true , wrap_key=false and without
wrap_key. And ensure that inline sizes are equal for all these cases. And add
coment that `PK index is always unwrapped if table was created via DDL`
--
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]
