jiangxt2 commented on code in PR #12274:
URL: https://github.com/apache/gravitino/pull/12274#discussion_r3793551331
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/ClickHouseTablePropertiesMetadata.java:
##########
@@ -99,6 +99,15 @@ public class ClickHouseTablePropertiesMetadata extends
JdbcTablePropertiesMetada
"",
false);
+ public static final PropertyEntry<String> ENGINE_PARAMETERS_PROPERTY_ENTRY =
+ stringOptionalPropertyEntry(
+ TableConstants.ENGINE_PARAMETERS,
+ "Engine parameters extracted from engine_full (e.g. \"ts\" for
ReplacingMergeTree(ts)). "
+ + "Only used for non-Distributed MergeTree-family engines.",
+ false,
+ "",
+ false);
Review Comment:
Fixed in 7517295a. The property description now states that
engine_parameters can be supplied during CREATE and restored by loadTable, and
is limited to the four supported MergeTree engines.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java:
##########
@@ -84,4 +84,92 @@ void testGetIndexesSqlEscapesSingleQuotes() throws Exception
{
primaryKeySql.contains("db''1"), "database single quote should be
doubled");
Assertions.assertTrue(primaryKeySql.contains("t''1"), "table single quote
should be doubled");
}
+
+ //
---------------------------------------------------------------------------
+ // 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() {
+ // Engine name appears later in the string — should not match because '('
check fails
Review Comment:
Fixed in 7517295a. The comment now correctly states that the method returns
null because the input does not start with the requested engine name.
--
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]
