yuqi1129 commented on code in PR #12274:
URL: https://github.com/apache/gravitino/pull/12274#discussion_r3748643621
##########
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())) {
Review Comment:
The scope check does not match the property contract. This extracts
parameters for every non-Distributed engine, including
MySQL/JDBC/S3/Kafka/Join/Buffer/File. `engine_full` for integration engines
contains connection arguments (and may contain or mask credentials), while
`ENGINE_PARAMETERS_PROPERTY_ENTRY` is public and non-hidden; the create path
also re-emits the value verbatim. This exposes engine configuration through
`table.properties()` and silently expands CREATE support beyond the intended
MergeTree family. Please gate both extraction and rendering on an explicit
supported MergeTree engine set, handle Graphite separately, and add a
non-MergeTree regression test.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -1642,6 +1661,43 @@ private String buildDataSkippingIndexDdl(
.formatted(quoteIdentifier(indexName), fieldStr, typeName,
granularity);
}
+ /**
+ * Extracts engine parameters from the {@code engine_full} column of {@code
system.tables}.
+ *
+ * <p>Uses bracket-depth counting to correctly handle nested parentheses
(e.g. {@code
+ * SummingMergeTree((a, b))} returns {@code "(a, b)"}). For {@code
ReplacingMergeTree(ts)},
+ * returns {@code "ts"}. For engines without parameters (e.g. {@code
MergeTree}), returns {@code
+ * null}.
+ */
+ @VisibleForTesting
+ static String extractEngineParams(String engineName, String engineFull) {
+ if (StringUtils.isBlank(engineFull) || StringUtils.isBlank(engineName)) {
+ return null;
+ }
+ if (!StringUtils.startsWithIgnoreCase(engineFull, engineName)) {
+ return null;
+ }
+ int paramsStart = engineName.length();
+ if (paramsStart >= engineFull.length() || engineFull.charAt(paramsStart)
!= '(') {
+ return null;
+ }
+ // Bracket-depth counting handles nested parentheses (e.g.
SummingMergeTree((a, b))).
+ int depth = 0;
+ for (int i = paramsStart + 1; i < engineFull.length(); i++) {
+ char ch = engineFull.charAt(i);
+ if (ch == '(') {
+ depth++;
+ } else if (ch == ')') {
Review Comment:
The depth scan is not quote-aware. For example, an engine parameter
containing a backtick-quoted identifier whose name is `ver)` will terminate at
the `)` inside the identifier and return a truncated value. Parentheses inside
string literals have the same problem. Please track
single-quoted/backtick-quoted and escaped state (or use a parser), and add
regression tests for quoted identifiers and strings.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -458,7 +458,12 @@ private ClickHouseTablePropertiesMetadata.ENGINE
appendTableEngine(
return engine;
}
- sqlBuilder.append("\n ENGINE = %s".formatted(engine.getValue()));
+ String engineParams =
StringUtils.trim(properties.get(TableConstants.ENGINE_PARAMETERS));
Review Comment:
GraphiteMergeTree still cannot round-trip through the new property. The
earlier Graphite branch reads only `graphite.config` and returns before this
generic path, but the load path stores the extracted value only as
`engine_parameters`. Therefore creating Graphite with the new public property
fails, and properties returned by `loadTable` cannot recreate the same engine.
Please keep Graphite on the dedicated `graphite.config` path by populating that
property (unquoted) during load and excluding Graphite from generic extraction,
or unify the two paths consistently. A SQL-generation round-trip unit test can
cover this without requiring a configured Graphite server.
--
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]
