Copilot commented on code in PR #12274:
URL: https://github.com/apache/gravitino/pull/12274#discussion_r3748805207
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -705,6 +710,20 @@ protected Map<String, String>
getTableProperties(Connection connection, String t
put(ClusterConstants.ON_CLUSTER, String.valueOf(false));
}
+ // Extract engine parameters from engine_full for
non-Distributed engines.
+ // engine_full contains the full engine DDL (e.g.
+ // "ReplacingMergeTree(ts) ORDER BY id SETTINGS ..."),
while the engine
+ // column only contains the engine name without parameters.
+ // For Distributed engines, parameters are already stored
as separate
+ // distributed-table properties below.
+ if (!StringUtils.equalsIgnoreCase(engine,
ENGINE.DISTRIBUTED.getValue())) {
+ String engineFull = resultSet.getString("engine_full");
+ String engineParams = extractEngineParams(engine,
engineFull);
+ if (StringUtils.isNotBlank(engineParams)) {
+ put(TableConstants.ENGINE_PARAMETERS, engineParams);
+ }
+ }
Review Comment:
GraphiteMergeTree tables will hit this non-Distributed branch and store
engine_parameters from engine_full, but GraphiteMergeTree creation currently
requires graphite.config (and ignores engine_parameters). This means a
load→create round-trip for GraphiteMergeTree still fails/misleads. Consider
also populating graphite.config from engine_full (or skipping engine_parameters
for GraphiteMergeTree until create supports it).
##########
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:
ENGINE_PARAMETERS_PROPERTY_ENTRY description says parameters are extracted
from engine_full, but this property is also user-settable and is used during
CREATE TABLE generation. Updating the description will avoid confusing API
users.
##########
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:
This test comment describes the failure reason as the '(' check, but the
implementation returns null earlier because engine_full does not start with the
engine name. Keeping the comment accurate makes the test intent clearer.
--
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]
