This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new af8d08bb3d [#12913] fix(clickhouse): preserve quoted commas in table
settings (#12918)
af8d08bb3d is described below
commit af8d08bb3d7c401c398b854ace8518f7b1cc2595
Author: StormSpirit <[email protected]>
AuthorDate: Sun Sep 20 20:18:33 2026 +0800
[#12913] fix(clickhouse): preserve quoted commas in table settings (#12918)
### What changes were proposed in this pull request?
This pull request fixes ClickHouse table-level SETTINGS readback when a
scalar literal contains commas.
- Replace the unconditional comma split in
`ClickHouseTableOperations.parseSettingsClause()` with top-level
delimiter parsing that ignores commas and equals signs inside quoted
text and nested parentheses.
- Locate the top-level `SETTINGS` clause while ignoring quoted text and
engine parameters, and preserve the complete remainder of the
server-returned `engine_full` value.
- Preserve the complete server-returned scalar-literal text and the
existing `settings.*` property representation, including the current
last-value-wins behavior for duplicate keys.
- Reuse the existing ClickHouse quote and parenthesis scanning behavior
for backslash escapes, doubled quote delimiters, and quoted identifiers.
- Reject structurally malformed SETTINGS metadata with a
setting-specific error that does not include the raw value.
- Add focused unit coverage and a real ClickHouse native-load and
load/recreate regression test.
No public API, property key, dependency, documentation, ALTER behavior,
or general SQL parsing capability is changed.
### Why are the changes needed?
The ClickHouse catalog reads table-level settings from
`system.tables.engine_full`, but the current parser splits the SETTINGS
clause at every comma. A valid String setting such as `merge_workload =
'gravitino,quoted,comma'` is therefore loaded as only `'gravitino`, even
though ClickHouse preserves the complete literal. Recreating a table
from the loaded property cannot retain the original value.
The existing write path already treats each `settings.*` value as one
complete ClickHouse scalar literal, so this change restores read/write
symmetry without changing the property contract.
Fix: #12913
### Does this PR introduce _any_ user-facing change?
Yes. Loading a ClickHouse table now preserves complete table-level
setting values when quoted text or nested function-style values contain
commas. Existing simple settings and `settings.*` property keys are
unchanged. Structurally malformed SETTINGS metadata now fails explicitly
instead of being silently ignored or converted into truncated
properties.
### How was this patch tested?
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:spotlessCheck` —
passed on the current revision `e71a4518`.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test --tests
'org.apache.gravitino.catalog.clickhouse.operations.TestClickHouseTableOperationsUnit'
-PskipITs` — passed on the current revision, covering quoted `COMMENT`
values, quoted engine parameters containing `SETTINGS`, malformed
metadata before the SETTINGS clause, escaping, doubled quotes, nested
parentheses, and duplicate keys.
- `git diff --check` — passed.
- Docker-tagged ClickHouse integration tests were not rerun after the
review fixes because the local macOS `docker-connector` was unavailable;
no current-revision Docker IT result is claimed.
---------
Signed-off-by: jiangxt2 <[email protected]>
---
.../operations/ClickHouseTableOperations.java | 162 ++++++++++++++-------
.../integration/test/CatalogClickHouseIT.java | 64 +++++++-
.../TestClickHouseTableOperationsUnit.java | 108 ++++++++++++++
3 files changed, 281 insertions(+), 53 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 55f4ad5d59..21f824a9fe 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
@@ -100,6 +100,8 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
private static final String CLICKHOUSE_NOT_SUPPORT_NESTED_COLUMN_MSG =
"Clickhouse does not support nested column names.";
+ private static final String INVALID_SETTINGS_METADATA_MSG =
+ "Invalid ClickHouse table SETTINGS metadata";
/** Default GRANULARITY for data skipping indexes, matching ClickHouse's own
default. */
private static final long DEFAULT_INDEX_GRANULARITY = 1;
@@ -110,8 +112,6 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
ENGINE.SUMMINGMERGETREE,
ENGINE.COLLAPSINGMERGETREE,
ENGINE.VERSIONEDCOLLAPSINGMERGETREE));
- private static final Pattern SETTINGS_PATTERN =
- Pattern.compile("(?is)\\bSETTINGS\\s+(.+?)(?=\\bCOMMENT\\b|$)");
private static final Pattern DISTRIBUTED_ENGINE_PATTERN =
Pattern.compile(
"(?i)^Distributed\\(([^,]+),\\s*([^,]+),\\s*([^,]+),\\s*(.+)\\)$",
Pattern.DOTALL);
@@ -1612,34 +1612,98 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
return ClickHouseTableSqlUtils.parsePartitioning(partitionKey);
}
- // Parses "key1 = val1, key2 = val2" from a SETTINGS clause.
- // Keys are prefixed with "settings." to match the write path convention in
- // appendTableProperties(). ClickHouse SETTINGS values are scalar (UInt64,
Bool,
- // String, Enum) — arrays or nested structures are not valid SETTINGS values,
- // so splitting by comma is safe.
+ // Parses "key1 = val1, key2 = val2" from a SETTINGS clause. Keys are
prefixed with
+ // "settings." to match the write path convention in appendTableProperties().
private static Map<String, String> parseSettingsClause(String settingsStr) {
Map<String, String> settings = new HashMap<>();
- for (String pair : settingsStr.split(",")) {
- String trimmed = pair.trim();
- int eqIdx = trimmed.indexOf('=');
- if (eqIdx > 0) {
- String key = trimmed.substring(0, eqIdx).trim();
- String value = trimmed.substring(eqIdx + 1).trim();
- settings.put(TableConstants.SETTINGS_PREFIX + key, value);
+ int fragmentStart = 0;
+ int equalsIndex = -1;
+ for (int i = 0; i < settingsStr.length(); i++) {
+ char current = settingsStr.charAt(i);
+ if (isQuoteDelimiter(current)) {
+ int quoteEnd = findClosingQuote(settingsStr, i);
+ Preconditions.checkArgument(quoteEnd >= 0,
INVALID_SETTINGS_METADATA_MSG);
+ i = quoteEnd;
+ } else if (current == '(') {
+ int parenthesisEnd = findMatchingParenthesis(settingsStr, i);
+ Preconditions.checkArgument(parenthesisEnd >= 0,
INVALID_SETTINGS_METADATA_MSG);
+ i = parenthesisEnd;
+ } else if (current == ')') {
+ throw new IllegalArgumentException(INVALID_SETTINGS_METADATA_MSG);
+ } else if (current == '=' && equalsIndex < 0) {
+ equalsIndex = i;
+ } else if (current == ',') {
+ addSetting(settings, settingsStr, fragmentStart, equalsIndex, i);
+ fragmentStart = i + 1;
+ equalsIndex = -1;
}
}
+
+ addSetting(settings, settingsStr, fragmentStart, equalsIndex,
settingsStr.length());
return settings;
}
+ private static void addSetting(
+ Map<String, String> settings,
+ String settingsStr,
+ int fragmentStart,
+ int equalsIndex,
+ int fragmentEnd) {
+ Preconditions.checkArgument(
+ equalsIndex >= fragmentStart && equalsIndex < fragmentEnd,
INVALID_SETTINGS_METADATA_MSG);
+ String key = settingsStr.substring(fragmentStart, equalsIndex).trim();
+ String value = settingsStr.substring(equalsIndex + 1, fragmentEnd).trim();
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(key) && StringUtils.isNotBlank(value),
+ INVALID_SETTINGS_METADATA_MSG);
+ settings.put(TableConstants.SETTINGS_PREFIX + key, value);
+ }
+
+ private static int findLastTopLevelKeyword(String value, String keyword) {
+ int keywordIndex = -1;
+ for (int i = 0; i < value.length(); i++) {
+ char current = value.charAt(i);
+ if (isQuoteDelimiter(current)) {
+ int quoteEnd = findClosingQuote(value, i);
+ Preconditions.checkArgument(quoteEnd >= 0,
INVALID_SETTINGS_METADATA_MSG);
+ i = quoteEnd;
+ } else if (current == '(') {
+ int parenthesisEnd = findMatchingParenthesis(value, i);
+ Preconditions.checkArgument(parenthesisEnd >= 0,
INVALID_SETTINGS_METADATA_MSG);
+ i = parenthesisEnd;
+ } else if (current == ')') {
+ throw new IllegalArgumentException(INVALID_SETTINGS_METADATA_MSG);
+ } else if (isKeywordAt(value, i, keyword)) {
+ keywordIndex = i;
+ }
+ }
+ return keywordIndex;
+ }
+
+ private static boolean isKeywordAt(String value, int index, String keyword) {
+ int keywordEnd = index + keyword.length();
+ return keywordEnd <= value.length()
+ && value.regionMatches(true, index, keyword, 0, keyword.length())
+ && (index == 0 || !isIdentifierCharacter(value.charAt(index - 1)))
+ && (keywordEnd == value.length() ||
!isIdentifierCharacter(value.charAt(keywordEnd)));
+ }
+
+ private static boolean isIdentifierCharacter(char value) {
+ return Character.isLetterOrDigit(value) || value == '_';
+ }
+
@VisibleForTesting
Map<String, String> parseSettingsFromEngineFull(String engineFull) {
if (StringUtils.isBlank(engineFull)) {
return Collections.emptyMap();
}
- Matcher settingsMatcher = SETTINGS_PATTERN.matcher(engineFull);
- if (settingsMatcher.find()) {
- return parseSettingsClause(settingsMatcher.group(1));
+ // engine_full is formatted from ClickHouse's ASTStorage, where SETTINGS
is the final storage
+ // clause. Use the last top-level match because ORDER BY may contain an
unquoted identifier
+ // named "settings". Quoted values and engine parameters are skipped by
the scanner.
+ int settingsStart = findLastTopLevelKeyword(engineFull, "SETTINGS");
+ if (settingsStart >= 0) {
+ return parseSettingsClause(engineFull.substring(settingsStart +
"SETTINGS".length()).trim());
}
return Collections.emptyMap();
}
@@ -2258,48 +2322,21 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
private static boolean isSingleQuotedLiteral(@Nullable String value) {
String literal = StringUtils.trim(value);
- if (StringUtils.length(literal) < 2 || literal.charAt(0) != '\'') {
- return false;
- }
-
- for (int i = 1; i < literal.length(); i++) {
- char current = literal.charAt(i);
- if (current == '\\') {
- if (i + 1 >= literal.length()) {
- return false;
- }
- i++;
- } else if (current == '\'') {
- if (i + 1 < literal.length() && literal.charAt(i + 1) == '\'') {
- i++;
- } else {
- return i == literal.length() - 1;
- }
- }
- }
- return false;
+ return StringUtils.length(literal) >= 2
+ && literal.charAt(0) == '\''
+ && findClosingQuote(literal, 0) == literal.length() - 1;
}
private static int findMatchingParenthesis(String value, int
openParenthesis) {
int depth = 1;
- char quote = 0;
for (int i = openParenthesis + 1; i < value.length(); i++) {
char current = value.charAt(i);
- if (quote != 0) {
- if (current == '\\' && i + 1 < value.length()) {
- i++;
- } else if (current == quote) {
- if (i + 1 < value.length() && value.charAt(i + 1) == quote) {
- i++;
- } else {
- quote = 0;
- }
+ if (isQuoteDelimiter(current)) {
+ int quoteEnd = findClosingQuote(value, i);
+ if (quoteEnd < 0) {
+ return -1;
}
- continue;
- }
-
- if (current == '\'' || current == '"' || current == '`') {
- quote = current;
+ i = quoteEnd;
} else if (current == '(') {
depth++;
} else if (current == ')') {
@@ -2312,6 +2349,27 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
return -1;
}
+ private static int findClosingQuote(String value, int openQuote) {
+ char quote = value.charAt(openQuote);
+ for (int i = openQuote + 1; i < value.length(); i++) {
+ char current = value.charAt(i);
+ if (current == '\\' && i + 1 < value.length()) {
+ i++;
+ } else if (current == quote) {
+ if (i + 1 < value.length() && value.charAt(i + 1) == quote) {
+ i++;
+ } else {
+ return i;
+ }
+ }
+ }
+ return -1;
+ }
+
+ private static boolean isQuoteDelimiter(char value) {
+ return value == '\'' || value == '"' || value == '`';
+ }
+
private StringBuilder appendColumnDefinition(JdbcColumn column,
StringBuilder sqlBuilder) {
// Add Nullable data type
String dataType = typeConverter.fromGravitino(column.dataType());
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 ee2824733a..769fd71906 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
@@ -3011,10 +3011,19 @@ public class CatalogClickHouseIT extends BaseIT {
String name = GravitinoITUtils.genRandomName("settings_native");
clickhouseService.executeQuery(
String.format(
- "CREATE TABLE `%s`.`%s` (id Int32) ENGINE = MergeTree ORDER BY id"
+ "CREATE TABLE `%s`.`%s` (settings Int32) ENGINE = MergeTree ORDER
BY settings"
+ " SETTINGS index_granularity = 2048",
schemaName, name));
+ String engineFull =
+ clickhouseService.executeQueryForResult(
+ String.format(
+ "SELECT engine_full FROM system.tables WHERE database = '%s'
AND name = '%s'",
+ schemaName, name));
+ Assertions.assertNotNull(engineFull);
+ Assertions.assertTrue(
+ engineFull.contains("ORDER BY settings SETTINGS index_granularity =
2048"), engineFull);
+
Table loaded =
catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName, name));
Map<String, String> props = loaded.properties();
Assertions.assertEquals(
@@ -3025,6 +3034,59 @@ public class CatalogClickHouseIT extends BaseIT {
Assertions.assertEquals(1, settingsCount);
}
+ @Test
+ void testLoadAndRecreateTableWithQuotedCommaSetting() {
+ String sourceName =
GravitinoITUtils.genRandomName("settings_quoted_comma_source");
+ String recreatedName =
GravitinoITUtils.genRandomName("settings_quoted_comma_recreated");
+ String settingName = "merge_workload";
+ String settingValue = "'gravitino,COMMENT,comma'";
+ String settingProperty = TableConstants.SETTINGS_PREFIX + settingName;
+
+ clickhouseService.executeQuery(
+ String.format(
+ "CREATE TABLE `%s`.`%s` (id Int32) ENGINE = MergeTree ORDER BY id"
+ + " SETTINGS %s = %s",
+ schemaName, sourceName, settingName, settingValue));
+
+ String sourceEngineFull =
+ clickhouseService.executeQueryForResult(
+ String.format(
+ "SELECT engine_full FROM system.tables WHERE database = '%s'
AND name = '%s'",
+ schemaName, sourceName));
+ Assertions.assertNotNull(sourceEngineFull);
+ Assertions.assertTrue(
+ sourceEngineFull.contains(settingName + " = " + settingValue),
sourceEngineFull);
+
+ Table loadedSource =
+ catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName,
sourceName));
+ Assertions.assertEquals(settingValue,
loadedSource.properties().get(settingProperty));
+
+ Map<String, String> recreatedProperties = createProperties();
+ recreatedProperties.put(settingProperty,
loadedSource.properties().get(settingProperty));
+ catalog
+ .asTableCatalog()
+ .createTable(
+ NameIdentifier.of(schemaName, recreatedName),
+ loadedSource.columns(),
+ "quoted comma setting roundtrip",
+ recreatedProperties,
+ Distributions.NONE,
+ getSortOrders("id"));
+
+ Table loadedRecreated =
+ catalog.asTableCatalog().loadTable(NameIdentifier.of(schemaName,
recreatedName));
+ Assertions.assertEquals(settingValue,
loadedRecreated.properties().get(settingProperty));
+
+ String recreatedEngineFull =
+ clickhouseService.executeQueryForResult(
+ String.format(
+ "SELECT engine_full FROM system.tables WHERE database = '%s'
AND name = '%s'",
+ schemaName, recreatedName));
+ Assertions.assertNotNull(recreatedEngineFull);
+ Assertions.assertTrue(
+ recreatedEngineFull.contains(settingName + " = " + settingValue),
recreatedEngineFull);
+ }
+
@Test
void testCreateTableWithSettingsViaGravitinoApi() {
String name = GravitinoITUtils.genRandomName("settings_api");
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 fbbffcca15..e5688697bc 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
@@ -469,6 +469,114 @@ public class TestClickHouseTableOperationsUnit {
"0", settings.get(TableConstants.SETTINGS_PREFIX +
"min_bytes_for_wide_part"));
}
+ @Test
+ void testParseSettingsPreservesQuotedAndNestedValues() {
+ ExposedClickHouseTableOperations ops = newOps();
+ Map<String, String> settings =
+ ops.parseSettingsFromEngineFull(
+ "MergeTree ORDER BY id SETTINGS "
+ + "quoted = 'id,COMMENT,name,val', "
+ + "escaped = 'a\\'b\\\\c', "
+ + "doubled = 'a''b,c', "
+ + "double_quoted = \"a,b\"\"c\", "
+ + "nested = custom(`a,b`, tuple(1, 2), 'x=y,z')");
+
+ Assertions.assertEquals(5, settings.size());
+ Assertions.assertEquals(
+ "'id,COMMENT,name,val'", settings.get(TableConstants.SETTINGS_PREFIX +
"quoted"));
+ Assertions.assertEquals(
+ "'a\\'b\\\\c'", settings.get(TableConstants.SETTINGS_PREFIX +
"escaped"));
+ Assertions.assertEquals("'a''b,c'",
settings.get(TableConstants.SETTINGS_PREFIX + "doubled"));
+ Assertions.assertEquals(
+ "\"a,b\"\"c\"", settings.get(TableConstants.SETTINGS_PREFIX +
"double_quoted"));
+ Assertions.assertEquals(
+ "custom(`a,b`, tuple(1, 2), 'x=y,z')",
+ settings.get(TableConstants.SETTINGS_PREFIX + "nested"));
+ }
+
+ @Test
+ void testParseSettingsFindsLastTopLevelClause() {
+ ExposedClickHouseTableOperations ops = newOps();
+
+ Map<String, String> settings =
+ ops.parseSettingsFromEngineFull(
+ "ReplacingMergeTree(`SETTINGS version`) ORDER BY id SETTINGS
index_granularity = 8192");
+ Assertions.assertEquals(1, settings.size());
+ Assertions.assertEquals(
+ "8192", settings.get(TableConstants.SETTINGS_PREFIX +
"index_granularity"));
+
+ settings =
+ ops.parseSettingsFromEngineFull(
+ "ReplicatedMergeTree('path SETTINGS ignored') ORDER BY id "
+ + "SETTINGS index_granularity = 4096");
+ Assertions.assertEquals(1, settings.size());
+ Assertions.assertEquals(
+ "4096", settings.get(TableConstants.SETTINGS_PREFIX +
"index_granularity"));
+
+ settings =
+ ops.parseSettingsFromEngineFull(
+ "MergeTree ORDER BY settings SETTINGS index_granularity = 8192");
+ Assertions.assertEquals(1, settings.size());
+ Assertions.assertEquals(
+ "8192", settings.get(TableConstants.SETTINGS_PREFIX +
"index_granularity"));
+ }
+
+ @Test
+ void testParseSettingsRejectsMalformedMetadataBeforeClause() {
+ ExposedClickHouseTableOperations ops = newOps();
+ String[] malformedEngineFull = {
+ "MergeTree(broken SETTINGS index_granularity = 8192",
+ "MergeTree('broken SETTINGS index_granularity = 8192",
+ "MergeTree ORDER BY 'oops SETTINGS index_granularity = 1",
+ "MergeTree() ORDER BY id) SETTINGS index_granularity = 8192"
+ };
+
+ for (String engineFull : malformedEngineFull) {
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
ops.parseSettingsFromEngineFull(engineFull));
+ Assertions.assertEquals("Invalid ClickHouse table SETTINGS metadata",
exception.getMessage());
+ }
+ }
+
+ @Test
+ void testParseSettingsPreservesLastDuplicateValue() {
+ Map<String, String> settings =
+ newOps()
+ .parseSettingsFromEngineFull(
+ "MergeTree ORDER BY id SETTINGS duplicate = 1, duplicate =
'last,value'");
+
+ Assertions.assertEquals(1, settings.size());
+ Assertions.assertEquals(
+ "'last,value'", settings.get(TableConstants.SETTINGS_PREFIX +
"duplicate"));
+ }
+
+ @Test
+ void testParseSettingsRejectsStructurallyInvalidMetadata() {
+ ExposedClickHouseTableOperations ops = newOps();
+ String[] invalidSettings = {
+ "missing_equals",
+ "= 1",
+ "key = ",
+ ", key = 1",
+ "key = 1,",
+ "key = 1,, other = 2",
+ "key = 'unterminated",
+ "key = custom(1, 2",
+ "key = value)"
+ };
+
+ for (String invalidSetting : invalidSettings) {
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.parseSettingsFromEngineFull(
+ "MergeTree ORDER BY id SETTINGS " + invalidSetting));
+ Assertions.assertEquals("Invalid ClickHouse table SETTINGS metadata",
exception.getMessage());
+ }
+ }
+
@Test
void testGetSystemTableMetadataThrowsWhenTableIsNotVisible() throws
Exception {
ExposedClickHouseTableOperations ops = newOps();
