This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-8d60f23a-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit a7de36d99516d32c83ebb390518d56927045bf3b Author: StormSpirit <[email protected]> AuthorDate: Thu Aug 27 22:22:39 2026 +0800 [#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016) ### What changes were proposed in this pull request? When a ClickHouse table contains a PROJECTION with its own ORDER BY clause, Gravitino parses the wrong ORDER BY from the SHOW CREATE TABLE output and returns incorrect sort keys. This PR strips PROJECTION definition blocks from the DDL before applying the existing regex patterns, so the table-level ORDER BY / PARTITION BY / SETTINGS clauses are matched correctly. ### Why are the changes needed? A PROJECTION block sits inside the column-definition body of the DDL: ```sql CREATE TABLE t ( `id` Int64, PROJECTION p_normal ( SELECT * ORDER BY dt ), `dt` Date ) ENGINE = MergeTree ORDER BY (id, dt) ``` The projection-internal `ORDER BY dt` appears before the table-level `ORDER BY (id, dt)`, so `find()` matches the wrong one. This affects any table with a Normal projection (projections with no ORDER BY, like Aggregate projections with only GROUP BY, are not affected). Fixes #11972 ### Does this PR introduce _any_ user-facing change? No. Tables without projections are unaffected. For tables with projections, `sortOrder()` now returns the correct table-level sort keys instead of garbled projection-internal content. ### How was this patch tested? - Unit tests: 19 new tests covering Normal/Aggregate/Multiple projections, INDEX coexistence, projection at different positions, string literals containing "PROJECTION", and escaped string literals inside projection bodies. - Docker integration tests: 11 tests on real ClickHouse 24.8 covering inline CREATE, ALTER TABLE ADD PROJECTION, PARTITION BY + PRIMARY KEY, ReplacingMergeTree, complex sort keys, and projections at all positions. - Existing tests (SETTINGS, INDEX, type mapping) continue to pass. --------- Signed-off-by: jiangxt2 <[email protected]> Co-authored-by: Qi Yu <[email protected]> # Conflicts: # catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java # catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java # catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java --- .../operations/ClickHouseTableOperations.java | 93 +++-- .../integration/test/CatalogClickHouseIT.java | 305 ++++++++++++++++- .../operations/TestClickHouseTableOperations.java | 58 +--- .../TestClickHouseTableOperationsUnit.java | 378 ++++++++++++++++++++- 4 files changed, 763 insertions(+), 71 deletions(-) diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java index 7da8809e1f..ca624f871a 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java @@ -92,9 +92,19 @@ public class ClickHouseTableOperations extends JdbcTableOperations { /** Default GRANULARITY for data skipping indexes, matching ClickHouse's own default. */ private static final long DEFAULT_INDEX_GRANULARITY = 1; +<<<<<<< HEAD private static final Pattern ORDER_BY_PATTERN = Pattern.compile( "(?is)\\bORDER\\s+BY\\s*(.+?)(?=\\bPARTITION\\s+BY\\b|\\bPRIMARY\\s+KEY\\b|\\bSAMPLE\\s+BY\\b|\\bTTL\\b|\\bSETTINGS\\b|\\bCOMMENT\\b|$)"); +======= + private static final Set<ENGINE> GENERIC_ENGINE_PARAMETER_ENGINES = + Collections.unmodifiableSet( + EnumSet.of( + ENGINE.REPLACINGMERGETREE, + ENGINE.SUMMINGMERGETREE, + ENGINE.COLLAPSINGMERGETREE, + ENGINE.VERSIONEDCOLLAPSINGMERGETREE)); +>>>>>>> 8d60f23a5 ([#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016)) private static final Pattern PARTITION_BY_PATTERN = Pattern.compile( "(?is)\\bPARTITION\\s+BY\\s*(.+?)(?=\\bORDER\\s+BY\\b|\\bPRIMARY\\s+KEY\\b|\\bSAMPLE\\s+BY\\b|\\bTTL\\b|\\bSETTINGS\\b|\\bCOMMENT\\b|$)"); @@ -782,25 +792,28 @@ public class ClickHouseTableOperations extends JdbcTableOperations { List<Index> indexes = getIndexes(connection, databaseName, tableName); jdbcTableBuilder.withIndexes(indexes.toArray(new Index[0])); - ShowCreateTableMetadata metadata = parseShowCreateTable(connection, tableName); - Transform[] partitioning = metadata.partitioning; + SystemTableMetadata systemTableMetadata = + getSystemTableMetadata(connection, databaseName, tableName); + ShowCreateTableMetadata showCreateMetadata = parseShowCreateTable(connection, tableName); + Transform[] partitioning = showCreateMetadata.partitioning; if (ArrayUtils.isEmpty(partitioning)) { partitioning = getTablePartitioning(connection, databaseName, tableName); } jdbcTableBuilder.withPartitioning(partitioning); - jdbcTableBuilder.withSortOrders(metadata.sortOrders); + jdbcTableBuilder.withSortOrders(systemTableMetadata.sortOrders()); Distribution distribution = getDistributionInfo(connection, databaseName, tableName); jdbcTableBuilder.withDistribution(distribution); Map<String, String> tableProperties = getTableProperties(connection, tableName); - // Merge SETTINGS parsed from SHOW CREATE TABLE into table properties. - // SHOW CREATE TABLE is the authoritative source for SETTINGS; it takes precedence + // Merge SETTINGS parsed from system.tables.engine_full into table properties. + // engine_full contains only table-level storage clauses, so projection SETTINGS cannot be + // mistaken for table SETTINGS. These values take precedence // over any settings.* keys that might exist in system.tables (though getTableProperties() // currently does not read SETTINGS from system.tables, so no overlap occurs in practice). - if (!metadata.settings.isEmpty()) { + if (!systemTableMetadata.settings().isEmpty()) { Map<String, String> merged = new HashMap<>(tableProperties); - merged.putAll(metadata.settings); + merged.putAll(systemTableMetadata.settings()); tableProperties = Collections.unmodifiableMap(merged); } jdbcTableBuilder.withProperties(tableProperties); @@ -830,6 +843,26 @@ public class ClickHouseTableOperations extends JdbcTableOperations { return kinds; } + @VisibleForTesting + SystemTableMetadata getSystemTableMetadata( + Connection connection, String databaseName, String tableName) throws SQLException { + String sql = + "SELECT sorting_key, engine_full FROM system.tables WHERE database = ? AND name = ?"; + try (PreparedStatement statement = connection.prepareStatement(sql)) { + statement.setString(1, databaseName); + statement.setString(2, tableName); + try (ResultSet resultSet = statement.executeQuery()) { + if (resultSet.next()) { + return new SystemTableMetadata( + parseOrderByClause(resultSet.getString("sorting_key")), + parseSettingsFromEngineFull(resultSet.getString("engine_full"))); + } + } + } + + throw new NoSuchTableException("Table %s does not exist in %s.", tableName, databaseName); + } + @Override protected Transform[] getTablePartitioning( Connection connection, String databaseName, String tableName) throws SQLException { @@ -1382,21 +1415,11 @@ public class ClickHouseTableOperations extends JdbcTableOperations { return metadata; } - Matcher orderMatcher = ORDER_BY_PATTERN.matcher(createSql); - if (orderMatcher.find()) { - metadata.sortOrders = parseOrderByClause(orderMatcher.group(1)); - } - Matcher partitionMatcher = PARTITION_BY_PATTERN.matcher(createSql); if (partitionMatcher.find()) { metadata.partitioning = parsePartitioning(partitionMatcher.group(1)); } - Matcher settingsMatcher = SETTINGS_PATTERN.matcher(createSql); - if (settingsMatcher.find()) { - metadata.settings = parseSettingsClause(settingsMatcher.group(1)); - } - return metadata; } @@ -1420,13 +1443,16 @@ public class ClickHouseTableOperations extends JdbcTableOperations { } @VisibleForTesting - SortOrder[] parseSortOrdersFromCreateSql(String createSql) { - return parseCreateStatement(createSql).sortOrders; - } + Map<String, String> parseSettingsFromEngineFull(String engineFull) { + if (StringUtils.isBlank(engineFull)) { + return Collections.emptyMap(); + } - @VisibleForTesting - Map<String, String> parseSettingsFromCreateSql(String createSql) { - return parseCreateStatement(createSql).settings; + Matcher settingsMatcher = SETTINGS_PATTERN.matcher(engineFull); + if (settingsMatcher.find()) { + return parseSettingsClause(settingsMatcher.group(1)); + } + return Collections.emptyMap(); } private ShowCreateTableMetadata parseShowCreateTable(Connection connection, String tableName) @@ -1545,10 +1571,27 @@ public class ClickHouseTableOperations extends JdbcTableOperations { return expression.toString(); } + @VisibleForTesting + static final class SystemTableMetadata { + private final SortOrder[] sortOrders; + private final Map<String, String> settings; + + private SystemTableMetadata(SortOrder[] sortOrders, Map<String, String> settings) { + this.sortOrders = sortOrders; + this.settings = settings; + } + + SortOrder[] sortOrders() { + return sortOrders; + } + + Map<String, String> settings() { + return settings; + } + } + private static final class ShowCreateTableMetadata { private Transform[] partitioning = Transforms.EMPTY_TRANSFORM; - private SortOrder[] sortOrders = SortOrders.NONE; - private Map<String, String> settings = Collections.emptyMap(); } @VisibleForTesting diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java index 5ffa5344c4..9070d1246e 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java @@ -428,8 +428,8 @@ public class CatalogClickHouseIT extends BaseIT { } @Test - void testLoadTableFromShowCreateParsing() { - String name = GravitinoITUtils.genRandomName("show_create_table"); + void testLoadTableMetadataFromNativeSql() { + String name = GravitinoITUtils.genRandomName("native_table_metadata"); clickhouseService.executeQuery( String.format( "CREATE TABLE `%s`.`%s` (\n" @@ -2872,4 +2872,305 @@ public class CatalogClickHouseIT extends BaseIT { getSortOrders("id"), Indexes.EMPTY_INDEXES)); } +<<<<<<< HEAD +======= + + @Test + void testLoadTableWithProjectionUsesTableSortKey() { + String name = GravitinoITUtils.genRandomName("proj_normal"); + clickhouseService.executeQuery( + String.format( + "CREATE TABLE `%s`.`%s` (\n" + + " `id` Int64,\n" + + " `dt` Date,\n" + + " `val` String,\n" + + " PROJECTION p_normal\n" + + " (\n" + + " SELECT *\n" + + " ORDER BY dt\n" + + " )\n" + + ")\n" + + "ENGINE = MergeTree\n" + + "ORDER BY (id, dt)\n" + + "SETTINGS index_granularity = 8192", + schemaName, name)); + + Table loaded = catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, name)); + SortOrder[] sortOrders = loaded.sortOrder(); + + Assertions.assertEquals(2, sortOrders.length); + Assertions.assertTrue( + sortOrders[0].expression() instanceof NamedReference, + "First sort key should be a named reference"); + Assertions.assertArrayEquals( + new String[] {"id"}, ((NamedReference) sortOrders[0].expression()).fieldName()); + Assertions.assertTrue(sortOrders[1].expression() instanceof NamedReference); + Assertions.assertArrayEquals( + new String[] {"dt"}, ((NamedReference) sortOrders[1].expression()).fieldName()); + Assertions.assertEquals( + "8192", loaded.properties().get(TableConstants.SETTINGS_PREFIX + "index_granularity")); + } + + @Test + void testEngineParametersRoundTrip() { + // Create a ReplacingMergeTree table with engine parameter via Gravitino API and verify + // engine_parameters is preserved on load (round-trip). + String name = GravitinoITUtils.genRandomName("engine_params_rpt"); + NameIdentifier ident = NameIdentifier.of(schemaName, name); + Column[] cols = + new Column[] { + Column.of("id", Types.IntegerType.get(), "id", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("ts", Types.IntegerType.get(), "version", false, false, DEFAULT_VALUE_NOT_SET) + }; + + Map<String, String> properties = createProperties(); + properties.put(GRAVITINO_ENGINE_KEY, REPLACINGMERGETREE.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "ts"); + + catalog + .asTableCatalog() + .createTable( + ident, + cols, + "Engine params round-trip test", + properties, + Distributions.NONE, + getSortOrders("id")); + + Table loaded = catalog.asTableCatalog().loadTable(ident); + Map<String, String> loadedProps = loaded.properties(); + Assertions.assertEquals( + "ts", + loadedProps.get(TableConstants.ENGINE_PARAMETERS), + "engine_parameters 'ts' should survive create→load round-trip"); + + // Verify the actual DDL in ClickHouse contains the engine parameter. + String createSql = + clickhouseService.executeQueryForResult( + String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName, name)); + Assertions.assertTrue( + createSql.contains("ReplacingMergeTree(ts)"), + "SHOW CREATE TABLE should contain ReplacingMergeTree(ts): " + createSql); + } + + @Test + void testEngineParametersNestedParensRoundTrip() { + // SummingMergeTree with multi-column tuple parameter, e.g. SummingMergeTree((a, b)). + String name = GravitinoITUtils.genRandomName("engine_params_nested"); + NameIdentifier ident = NameIdentifier.of(schemaName, name); + Column[] cols = + new Column[] { + Column.of("id", Types.IntegerType.get(), "id", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("a", Types.IntegerType.get(), "a", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("b", Types.IntegerType.get(), "b", false, false, DEFAULT_VALUE_NOT_SET) + }; + + Map<String, String> properties = createProperties(); + properties.put(GRAVITINO_ENGINE_KEY, SUMMINGMERGETREE.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "(a, b)"); + + catalog + .asTableCatalog() + .createTable( + ident, + cols, + "Nested parens round-trip", + properties, + Distributions.NONE, + getSortOrders("id")); + + Table loaded = catalog.asTableCatalog().loadTable(ident); + Map<String, String> loadedProps = loaded.properties(); + Assertions.assertEquals( + "(a, b)", + loadedProps.get(TableConstants.ENGINE_PARAMETERS), + "engine_parameters '(a, b)' should survive round-trip"); + + String createSql = + clickhouseService.executeQueryForResult( + String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName, name)); + Assertions.assertTrue( + createSql.contains("SummingMergeTree((a, b))"), + "SHOW CREATE TABLE should contain SummingMergeTree((a, b)): " + createSql); + } + + @Test + void testEngineParametersMultiParamRoundTrip() { + // VersionedCollapsingMergeTree(sign, ver) — multi-parameter engine round-trip. + String name = GravitinoITUtils.genRandomName("engine_params_multi"); + NameIdentifier ident = NameIdentifier.of(schemaName, name); + Column[] cols = + new Column[] { + Column.of("id", Types.IntegerType.get(), "id", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("sign", Types.ByteType.get(), "sign", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("ver", Types.IntegerType.get(), "ver", false, false, DEFAULT_VALUE_NOT_SET) + }; + + Map<String, String> properties = createProperties(); + properties.put(GRAVITINO_ENGINE_KEY, VERSIONEDCOLLAPSINGMERGETREE.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "sign, ver"); + + catalog + .asTableCatalog() + .createTable( + ident, + cols, + "Multi-param engine round-trip", + properties, + Distributions.NONE, + getSortOrders("id")); + + Table loaded = catalog.asTableCatalog().loadTable(ident); + Map<String, String> loadedProps = loaded.properties(); + Assertions.assertEquals( + "sign, ver", + loadedProps.get(TableConstants.ENGINE_PARAMETERS), + "engine_parameters 'sign, ver' should survive round-trip"); + + String createSql = + clickhouseService.executeQueryForResult( + String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName, name)); + Assertions.assertTrue( + createSql.contains("VersionedCollapsingMergeTree(sign, ver)"), + "SHOW CREATE TABLE should contain VersionedCollapsingMergeTree(sign, ver): " + createSql); + } + + @Test + void testEngineParamsLoadFromExistingTable() { + // Create a table directly in ClickHouse (bypass Gravitino) with engine parameters, + // then load via Gravitino and verify engine_parameters is extracted. + String name = GravitinoITUtils.genRandomName("engine_params_load"); + clickhouseService.executeQuery( + String.format( + "CREATE TABLE `%s`.`%s` (id Int32, sign Int8) " + + "ENGINE = CollapsingMergeTree(sign) ORDER BY id", + schemaName, name)); + + Table loaded = catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, name)); + Map<String, String> props = loaded.properties(); + Assertions.assertEquals( + "sign", + props.get(TableConstants.ENGINE_PARAMETERS), + "CollapsingMergeTree(sign) should have engine_parameters='sign'"); + } + + @Test + void testEngineParamsAbsentForParameterizedNonMergeTreeEngine() { + String name = GravitinoITUtils.genRandomName("engine_params_join"); + clickhouseService.executeQuery( + String.format( + "CREATE TABLE `%s`.`%s` (id Int32, payload String) " + "ENGINE = Join(ANY, LEFT, id)", + schemaName, name)); + + Table loaded = catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, name)); + Assertions.assertEquals(ENGINE.JOIN.getValue(), loaded.properties().get(GRAVITINO_ENGINE_KEY)); + Assertions.assertFalse( + loaded.properties().containsKey(TableConstants.ENGINE_PARAMETERS), + "Parameterized non-MergeTree engines must not expose engine_parameters"); + } + + @Test + void testEngineParamsAbsentForMergeTree() { + // MergeTree has no engine parameters — verify the property is not present. + String name = GravitinoITUtils.genRandomName("engine_params_none"); + NameIdentifier ident = NameIdentifier.of(schemaName, name); + Column[] cols = + new Column[] { + Column.of("id", Types.IntegerType.get(), "id", false, false, DEFAULT_VALUE_NOT_SET) + }; + + Map<String, String> properties = createProperties(); + properties.put(GRAVITINO_ENGINE_KEY, MERGETREE.getValue()); + + catalog + .asTableCatalog() + .createTable( + ident, cols, "No engine params", properties, Distributions.NONE, getSortOrders("id")); + + Table loaded = catalog.asTableCatalog().loadTable(ident); + Map<String, String> loadedProps = loaded.properties(); + Assertions.assertFalse( + loadedProps.containsKey(TableConstants.ENGINE_PARAMETERS), + "MergeTree should not have engine_parameters property"); + } + + @Test + void testEnumRoundTrip() { + // Create a table in ClickHouse with Enum8 and Enum16 columns + String tableName = GravitinoITUtils.genRandomName("enum_test"); + clickhouseService.executeQuery( + String.format( + "CREATE TABLE `%s`.`%s` (" + + "id Int32, " + + "status Enum8('active' = 1, 'inactive' = 2), " + + "priority Enum16('low' = 100, 'medium' = 200, 'high' = 300)" + + ") ENGINE = MergeTree ORDER BY id", + schemaName, tableName)); + + // Load through Gravitino and verify column types are ExternalType + Table loaded = catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, tableName)); + Column[] columns = loaded.columns(); + Assertions.assertEquals(3, columns.length); + + // Enum8 type verification + Type enum8Type = columns[1].dataType(); + Assertions.assertTrue( + enum8Type instanceof Types.ExternalType, + "Enum8 should map to ExternalType, but got: " + enum8Type.simpleString()); + Assertions.assertEquals( + normalizeEnumFormatting("Enum8('active' = 1, 'inactive' = 2)"), + normalizeEnumFormatting(((Types.ExternalType) enum8Type).catalogString())); + + // Enum16 type verification + Type enum16Type = columns[2].dataType(); + Assertions.assertTrue( + enum16Type instanceof Types.ExternalType, + "Enum16 should map to ExternalType, but got: " + enum16Type.simpleString()); + Assertions.assertEquals( + normalizeEnumFormatting("Enum16('low' = 100, 'medium' = 200, 'high' = 300)"), + normalizeEnumFormatting(((Types.ExternalType) enum16Type).catalogString())); + + // Round-trip: recreate table through Gravitino with the loaded schema + String rtTableName = GravitinoITUtils.genRandomName("enum_rt"); + catalog + .asTableCatalog() + .createTable( + NameIdentifier.of(schemaName, rtTableName), + columns, + "enum round-trip test", + createProperties(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + new SortOrder[] {SortOrders.of(NamedReference.field("id"), SortDirection.ASCENDING)}); + + // Verify types survived round-trip + Table rtLoaded = catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, rtTableName)); + Column[] rtColumns = rtLoaded.columns(); + Type rtEnum8 = rtColumns[1].dataType(); + Type rtEnum16 = rtColumns[2].dataType(); + Assertions.assertTrue( + rtEnum8 instanceof Types.ExternalType, "Enum8 should survive round-trip as ExternalType"); + Assertions.assertTrue( + rtEnum16 instanceof Types.ExternalType, "Enum16 should survive round-trip as ExternalType"); + Assertions.assertEquals( + normalizeEnumFormatting(((Types.ExternalType) enum8Type).catalogString()), + normalizeEnumFormatting(((Types.ExternalType) rtEnum8).catalogString())); + Assertions.assertEquals( + normalizeEnumFormatting(((Types.ExternalType) enum16Type).catalogString()), + normalizeEnumFormatting(((Types.ExternalType) rtEnum16).catalogString())); + + // Verify DDL on ClickHouse side contains full enum definitions + String createSql = + clickhouseService.executeQueryForResult( + String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName, rtTableName)); + Assertions.assertNotNull(createSql, "SHOW CREATE TABLE should return a result"); + String normalizedCreateSql = normalizeEnumFormatting(createSql); + Assertions.assertTrue( + normalizedCreateSql.contains("Enum8('active'=1,'inactive'=2)"), + "SHOW CREATE TABLE should contain Enum8 definition: " + createSql); + Assertions.assertTrue( + normalizedCreateSql.contains("Enum16('low'=100,'medium'=200,'high'=300)"), + "SHOW CREATE TABLE should contain Enum16 definition: " + createSql); + } +>>>>>>> 8d60f23a5 ([#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016)) } diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java index 909b5ec81a..5f3fdbd8ea 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java @@ -1362,44 +1362,20 @@ public class TestClickHouseTableOperations extends TestClickHouse { } @Test - void testParseSortOrdersFromMultilineShowCreateSql() { - TestableClickHouseTableOperations ops = new TestableClickHouseTableOperations(); - String showCreateSql = - """ - CREATE TABLE `t1` - ( - `id` Int32, - `event_time` DateTime - ) - ENGINE = MergeTree - ORDER BY - (`id`, toDate(`event_time`)) - SETTINGS index_granularity = 8192 - """; - - SortOrder[] sortOrders = ops.parseSortOrders(showCreateSql); - Assertions.assertEquals(2, sortOrders.length); - Assertions.assertTrue(sortOrders[0].expression() instanceof NamedReference); - Assertions.assertEquals("id", ((NamedReference) sortOrders[0].expression()).fieldName()[0]); - } - - @Test - void testParseSettingsFromCreateSql() { + void testParseSettingsFromEngineFull() { TestableClickHouseTableOperations ops = new TestableClickHouseTableOperations(); // Single setting - String sql1 = - "CREATE TABLE t1 (id Int32) ENGINE = MergeTree ORDER BY id SETTINGS index_granularity = 4096"; - Map<String, String> settings1 = ops.parseSettings(sql1); + String engineFull1 = "MergeTree ORDER BY id SETTINGS index_granularity = 4096"; + Map<String, String> settings1 = ops.parseSettings(engineFull1); Assertions.assertEquals(1, settings1.size()); Assertions.assertEquals( "4096", settings1.get(TableConstants.SETTINGS_PREFIX + "index_granularity")); // Multiple settings - String sql2 = - "CREATE TABLE t2 (id Int32) ENGINE = MergeTree ORDER BY id" - + " SETTINGS index_granularity = 4096, min_bytes_for_wide_part = 0"; - Map<String, String> settings2 = ops.parseSettings(sql2); + String engineFull2 = + "MergeTree ORDER BY id" + " SETTINGS index_granularity = 4096, min_bytes_for_wide_part = 0"; + Map<String, String> settings2 = ops.parseSettings(engineFull2); Assertions.assertEquals(2, settings2.size()); Assertions.assertEquals( "4096", settings2.get(TableConstants.SETTINGS_PREFIX + "index_granularity")); @@ -1407,15 +1383,15 @@ public class TestClickHouseTableOperations extends TestClickHouse { "0", settings2.get(TableConstants.SETTINGS_PREFIX + "min_bytes_for_wide_part")); // No SETTINGS clause - String sql3 = "CREATE TABLE t3 (id Int32) ENGINE = MergeTree ORDER BY id"; - Map<String, String> settings3 = ops.parseSettings(sql3); + String engineFull3 = "MergeTree ORDER BY id"; + Map<String, String> settings3 = ops.parseSettings(engineFull3); Assertions.assertTrue(settings3.isEmpty()); - // SETTINGS with COMMENT after - String sql4 = - "CREATE TABLE t4 (id Int32) ENGINE = MergeTree ORDER BY id" - + " SETTINGS index_granularity = 8192 COMMENT 'test'"; - Map<String, String> settings4 = ops.parseSettings(sql4); + // Engine arguments before SETTINGS + String engineFull4 = + "ReplicatedMergeTree('/path', '{replica}') ORDER BY id" + + " SETTINGS index_granularity = 8192"; + Map<String, String> settings4 = ops.parseSettings(engineFull4); Assertions.assertEquals(1, settings4.size()); Assertions.assertEquals( "8192", settings4.get(TableConstants.SETTINGS_PREFIX + "index_granularity")); @@ -1435,12 +1411,8 @@ public class TestClickHouseTableOperations extends TestClickHouse { tableName, columns, comment, properties, partitioning, distribution, indexes, sortOrders); } - SortOrder[] parseSortOrders(String createSql) { - return parseSortOrdersFromCreateSql(createSql); - } - - Map<String, String> parseSettings(String createSql) { - return parseSettingsFromCreateSql(createSql); + Map<String, String> parseSettings(String engineFull) { + return parseSettingsFromEngineFull(engineFull); } } diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java index e31362c782..ea2e9efe31 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java @@ -26,6 +26,16 @@ import java.util.List; import org.apache.gravitino.catalog.clickhouse.converter.ClickHouseColumnDefaultValueConverter; import org.apache.gravitino.catalog.clickhouse.converter.ClickHouseExceptionConverter; import org.apache.gravitino.catalog.clickhouse.converter.ClickHouseTypeConverter; +<<<<<<< HEAD +======= +import org.apache.gravitino.catalog.jdbc.JdbcColumn; +import org.apache.gravitino.exceptions.NoSuchTableException; +import org.apache.gravitino.rel.expressions.FunctionExpression; +import org.apache.gravitino.rel.expressions.NamedReference; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +>>>>>>> 8d60f23a5 ([#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016)) import org.apache.gravitino.rel.indexes.Index; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -39,6 +49,39 @@ public class TestClickHouseTableOperationsUnit { throws Exception { return getIndexes(connection, databaseName, tableName); } +<<<<<<< HEAD +======= + + SystemTableMetadata callGetSystemTableMetadata( + Connection connection, String databaseName, String tableName) throws Exception { + return getSystemTableMetadata(connection, databaseName, tableName); + } + + Map<String, String> callGetTableProperties(Connection connection, String tableName) + throws Exception { + return getTableProperties(connection, tableName); + } + + String callGenerateCreateTableSql(Map<String, String> properties) { + JdbcColumn[] columns = + new JdbcColumn[] { + JdbcColumn.builder() + .withName("id") + .withType(Types.IntegerType.get()) + .withNullable(false) + .build() + }; + return generateCreateTableSql( + "test_table", + columns, + "", + properties, + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + Indexes.EMPTY_INDEXES, + getSortOrders("id")); + } +>>>>>>> 8d60f23a5 ([#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016)) } private ExposedClickHouseTableOperations newOps() { @@ -78,10 +121,343 @@ public class TestClickHouseTableOperationsUnit { ops.callGetIndexes(connection, "db'1", "t'1"); - // First captured SQL is the primary-key QUERY_INDEXES_SQL (string-interpolated). String primaryKeySql = sqlCaptor.getAllValues().get(0); Assertions.assertTrue( primaryKeySql.contains("db''1"), "database single quote should be doubled"); Assertions.assertTrue(primaryKeySql.contains("t''1"), "table single quote should be doubled"); } +<<<<<<< HEAD +======= + + @Test + void testGetSystemTableMetadataQueriesExactTable() throws Exception { + ExposedClickHouseTableOperations ops = newOps(); + Connection connection = Mockito.mock(Connection.class); + PreparedStatement statement = Mockito.mock(PreparedStatement.class); + ResultSet resultSet = Mockito.mock(ResultSet.class); + ArgumentCaptor<String> sqlCaptor = ArgumentCaptor.forClass(String.class); + + Mockito.when(connection.prepareStatement(sqlCaptor.capture())).thenReturn(statement); + Mockito.when(statement.executeQuery()).thenReturn(resultSet); + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString("sorting_key")).thenReturn("id"); + Mockito.when(resultSet.getString("engine_full")).thenReturn("MergeTree ORDER BY id"); + + ClickHouseTableOperations.SystemTableMetadata metadata = + ops.callGetSystemTableMetadata(connection, "db_name", "table_name"); + + Assertions.assertEquals( + "SELECT sorting_key, engine_full FROM system.tables WHERE database = ? AND name = ?", + sqlCaptor.getValue()); + Mockito.verify(statement).setString(1, "db_name"); + Mockito.verify(statement).setString(2, "table_name"); + Assertions.assertEquals(1, metadata.sortOrders().length); + Assertions.assertEquals(NamedReference.field("id"), metadata.sortOrders()[0].expression()); + Assertions.assertTrue(metadata.settings().isEmpty()); + Mockito.verify(resultSet).close(); + Mockito.verify(statement).close(); + } + + @Test + void testGetSystemTableMetadataParsesCompoundAndFunctionExpressions() throws Exception { + ExposedClickHouseTableOperations ops = newOps(); + Connection connection = Mockito.mock(Connection.class); + PreparedStatement statement = Mockito.mock(PreparedStatement.class); + ResultSet resultSet = Mockito.mock(ResultSet.class); + + Mockito.when(connection.prepareStatement(Mockito.anyString())).thenReturn(statement); + Mockito.when(statement.executeQuery()).thenReturn(resultSet); + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString("sorting_key")).thenReturn("id, toDate(event_time)"); + Mockito.when(resultSet.getString("engine_full")).thenReturn("MergeTree ORDER BY id"); + + SortOrder[] sortOrders = + ops.callGetSystemTableMetadata(connection, "db_name", "table_name").sortOrders(); + + Assertions.assertEquals(2, sortOrders.length); + Assertions.assertEquals(NamedReference.field("id"), sortOrders[0].expression()); + Assertions.assertEquals( + FunctionExpression.of("toDate", NamedReference.field("event_time")), + sortOrders[1].expression()); + } + + @Test + void testGetSystemTableMetadataReturnsNoneForBlankValues() throws Exception { + ExposedClickHouseTableOperations ops = newOps(); + Connection connection = Mockito.mock(Connection.class); + PreparedStatement statement = Mockito.mock(PreparedStatement.class); + ResultSet resultSet = Mockito.mock(ResultSet.class); + + Mockito.when(connection.prepareStatement(Mockito.anyString())).thenReturn(statement); + Mockito.when(statement.executeQuery()).thenReturn(resultSet); + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString("sorting_key")).thenReturn(" "); + Mockito.when(resultSet.getString("engine_full")).thenReturn(" "); + + ClickHouseTableOperations.SystemTableMetadata metadata = + ops.callGetSystemTableMetadata(connection, "db_name", "table_name"); + + Assertions.assertArrayEquals(new SortOrder[0], metadata.sortOrders()); + Assertions.assertTrue(metadata.settings().isEmpty()); + } + + @Test + void testGetSystemTableMetadataParsesSettingsFromEngineFull() throws Exception { + ExposedClickHouseTableOperations ops = newOps(); + Connection connection = Mockito.mock(Connection.class); + PreparedStatement statement = Mockito.mock(PreparedStatement.class); + ResultSet resultSet = Mockito.mock(ResultSet.class); + + Mockito.when(connection.prepareStatement(Mockito.anyString())).thenReturn(statement); + Mockito.when(statement.executeQuery()).thenReturn(resultSet); + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString("sorting_key")).thenReturn("id"); + Mockito.when(resultSet.getString("engine_full")) + .thenReturn( + "MergeTree ORDER BY id SETTINGS index_granularity = 4096, " + + "min_bytes_for_wide_part = 0"); + + Map<String, String> settings = + ops.callGetSystemTableMetadata(connection, "db_name", "table_name").settings(); + + Assertions.assertEquals(2, settings.size()); + Assertions.assertEquals( + "4096", settings.get(TableConstants.SETTINGS_PREFIX + "index_granularity")); + Assertions.assertEquals( + "0", settings.get(TableConstants.SETTINGS_PREFIX + "min_bytes_for_wide_part")); + } + + @Test + void testGetSystemTableMetadataThrowsWhenTableIsNotVisible() throws Exception { + ExposedClickHouseTableOperations ops = newOps(); + Connection connection = Mockito.mock(Connection.class); + PreparedStatement statement = Mockito.mock(PreparedStatement.class); + ResultSet resultSet = Mockito.mock(ResultSet.class); + + Mockito.when(connection.prepareStatement(Mockito.anyString())).thenReturn(statement); + Mockito.when(statement.executeQuery()).thenReturn(resultSet); + Mockito.when(resultSet.next()).thenReturn(false); + + NoSuchTableException exception = + Assertions.assertThrows( + NoSuchTableException.class, + () -> ops.callGetSystemTableMetadata(connection, "db_name", "table_name")); + + Assertions.assertTrue(exception.getMessage().contains("table_name")); + Assertions.assertTrue(exception.getMessage().contains("db_name")); + } + + // --------------------------------------------------------------------------- + // extractEngineParams + // --------------------------------------------------------------------------- + + @Test + void testExtractEngineParamsWithParams() { + Assertions.assertEquals( + "ts", + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", + "ReplacingMergeTree(ts) ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsMultipleParams() { + Assertions.assertEquals( + "sign, ts", + ClickHouseTableOperations.extractEngineParams( + "VersionedCollapsingMergeTree", + "VersionedCollapsingMergeTree(sign, ts) ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsSingleParam() { + Assertions.assertEquals( + "sign", + ClickHouseTableOperations.extractEngineParams( + "CollapsingMergeTree", + "CollapsingMergeTree(sign) ORDER BY id SETTINGS index_granularity = 8192")); + Assertions.assertEquals( + "val", + ClickHouseTableOperations.extractEngineParams( + "SummingMergeTree", + "SummingMergeTree(val) ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsNoParams() { + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams( + "MergeTree", "MergeTree ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsBlankInput() { + Assertions.assertNull(ClickHouseTableOperations.extractEngineParams("MergeTree", null)); + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams(null, "MergeTree ORDER BY id")); + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams("", "MergeTree ORDER BY id")); + } + + @Test + void testExtractEngineParamsEngineNameNotAtStart() { + // The engine name must be at the start of engine_full. + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams("MergeTree", "something else MergeTree(x)")); + } + + @Test + void testExtractEngineParamsNestedParens() { + // SummingMergeTree((a, b)) — nested parentheses should be preserved. + Assertions.assertEquals( + "(a, b)", + ClickHouseTableOperations.extractEngineParams( + "SummingMergeTree", + "SummingMergeTree((a, b)) ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsGraphiteMergeTree() { + // The generic scanner preserves the quoted parameter for Graphite-specific decoding. + Assertions.assertEquals( + "'graphite_rollup'", + ClickHouseTableOperations.extractEngineParams( + "GraphiteMergeTree", + "GraphiteMergeTree('graphite_rollup') ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsAggregatingMergeTree() { + // AggregatingMergeTree has no parameters. + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams( + "AggregatingMergeTree", + "AggregatingMergeTree ORDER BY id SETTINGS index_granularity = 8192")); + } + + @Test + void testExtractEngineParamsIgnoresParenthesesInsideQuotes() { + Assertions.assertEquals( + "`ver)`", + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", "ReplacingMergeTree(`ver)`) ORDER BY id")); + Assertions.assertEquals( + "\"ver)\"", + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", "ReplacingMergeTree(\"ver)\") ORDER BY id")); + Assertions.assertEquals( + "'rollup(test)'", + ClickHouseTableOperations.extractEngineParams( + "GraphiteMergeTree", "GraphiteMergeTree('rollup(test)') ORDER BY id")); + } + + @Test + void testExtractEngineParamsHandlesEscapedQuotes() { + Assertions.assertEquals( + "`ver``)`", + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", "ReplacingMergeTree(`ver``)`) ORDER BY id")); + Assertions.assertEquals( + "'rollup\\')test'", + ClickHouseTableOperations.extractEngineParams( + "GraphiteMergeTree", "GraphiteMergeTree('rollup\\')test') ORDER BY id")); + } + + @Test + void testExtractEngineParamsAllowsWhitespaceBeforeParameters() { + Assertions.assertEquals( + "ts", + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", " ReplacingMergeTree \t (ts) ORDER BY id")); + } + + @Test + void testExtractEngineParamsRejectsUnclosedInput() { + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", "ReplacingMergeTree('ver)) ORDER BY id")); + Assertions.assertNull( + ClickHouseTableOperations.extractEngineParams( + "ReplacingMergeTree", "ReplacingMergeTree(tuple(ts) ORDER BY id")); + } + + @Test + void testGraphitePropertiesRoundTripThroughSqlGeneration() throws Exception { + Map<String, String> loadedProperties = + loadTableProperties( + ENGINE.GRAPHITEMERGETREE.getValue(), + "GraphiteMergeTree('graphite''rollup\\\\path') ORDER BY id"); + + Assertions.assertEquals( + "graphite'rollup\\path", loadedProperties.get(TableConstants.GRAPHITE_CONFIG)); + Assertions.assertFalse(loadedProperties.containsKey(TableConstants.ENGINE_PARAMETERS)); + + String createSql = newOps().callGenerateCreateTableSql(loadedProperties); + Assertions.assertTrue( + createSql.contains("ENGINE = GraphiteMergeTree('graphite''rollup\\\\path')"), createSql); + } + + @Test + void testNonMergeTreeLoadDoesNotExposeEngineParameters() throws Exception { + Map<String, String> loadedProperties = + loadTableProperties( + ENGINE.MySQL.getValue(), "MySQL('host:9000', 'database', 'table', 'user', 'secret')"); + + Assertions.assertFalse(loadedProperties.containsKey(TableConstants.ENGINE_PARAMETERS)); + Assertions.assertFalse( + loadedProperties.values().stream() + .anyMatch(value -> value != null && value.contains("secret"))); + } + + @Test + void testSupportedEngineParametersGenerateSql() { + Map<String, String> properties = new HashMap<>(); + properties.put("engine", ENGINE.SUMMINGMERGETREE.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "(id)"); + + String createSql = newOps().callGenerateCreateTableSql(properties); + Assertions.assertTrue(createSql.contains("ENGINE = SummingMergeTree((id))"), createSql); + } + + @Test + void testUnsupportedEnginesRejectGenericParameters() { + for (ENGINE engine : + List.of( + ENGINE.MERGETREE, + ENGINE.AGGREGATINGMERGETREE, + ENGINE.JOIN, + ENGINE.MySQL, + ENGINE.GRAPHITEMERGETREE, + ENGINE.DISTRIBUTED)) { + Map<String, String> properties = new HashMap<>(); + properties.put("engine", engine.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "sensitive_value"); + if (engine == ENGINE.GRAPHITEMERGETREE) { + properties.put(TableConstants.GRAPHITE_CONFIG, "graphite_rollup"); + } + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> newOps().callGenerateCreateTableSql(properties), + engine.getValue()); + Assertions.assertTrue(exception.getMessage().contains("engine_parameters")); + if (engine == ENGINE.GRAPHITEMERGETREE) { + Assertions.assertTrue(exception.getMessage().contains("graphite.config")); + } + } + } + + @Test + void testSupportedEngineParametersCannotEscapeEngineClause() { + Map<String, String> properties = new HashMap<>(); + properties.put("engine", ENGINE.REPLACINGMERGETREE.getValue()); + properties.put(TableConstants.ENGINE_PARAMETERS, "ts) SETTINGS index_granularity = 1"); + + IllegalArgumentException exception = + Assertions.assertThrows( + IllegalArgumentException.class, () -> newOps().callGenerateCreateTableSql(properties)); + Assertions.assertTrue(exception.getMessage().contains("balanced")); + } +>>>>>>> 8d60f23a5 ([#11972] fix(clickhouse): strip PROJECTION blocks before extracting sort key from SHOW CREATE TABLE (#12016)) }
