yuqi1129 commented on code in PR #12918:
URL: https://github.com/apache/gravitino/pull/12918#discussion_r4048193742
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -1468,34 +1468,97 @@ Transform[] parsePartitioning(@Nullable String
partitionKey) {
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 findTopLevelKeyword(String value, String keyword) {
+ 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)) {
+ return i;
+ }
+ }
+ return -1;
+ }
+
+ 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. Locate it at top level and parse the remainder so keywords in
engine parameters and
+ // quoted values are not treated as clause boundaries.
+ int settingsStart = findTopLevelKeyword(engineFull, "SETTINGS");
Review Comment:
[P2] Keep a top-level `COMMENT` boundary after `SETTINGS`
The old pattern stopped the clause at `\bCOMMENT\b`; this now hands
everything after `SETTINGS` to `parseSettingsClause`. I agree that
`engine_full` is formatted from `ASTStorage` and the table comment lives on
`ASTCreateQuery`, so today SETTINGS is the last clause. But if any server
version does append `COMMENT '...'` here, the result is not an error but a
silently corrupted value: `index_granularity = 8192 COMMENT 'x'` parses as one
fragment with value `8192 COMMENT 'x'`, and a recreate would send that back to
ClickHouse.
A cheap guard keeps the old behaviour without the old regex's weakness: run
`findTopLevelKeyword(remainder, "COMMENT")` on the text after `SETTINGS` and
cut there when found. Quoted `COMMENT` inside a value is already skipped by the
top-level scan, so `'id,COMMENT,name,val'` in the new test stays intact.
Related nit: `findTopLevelKeyword` returns the first top-level match. A
column literally named `settings` is emitted unquoted by `backQuoteIfNeed`, so
`ORDER BY settings SETTINGS index_granularity = 8192` starts parsing at the
column and yields the key `SETTINGS index_granularity`. The old regex had the
same edge, but since SETTINGS is the final storage clause, taking the last
top-level match fixes it for free.
The quote scanning itself (`findClosingQuote` with backslash and
doubled-delimiter escapes for all three quote characters) matches the
ClickHouse lexer and what `quoteString`/`backQuoteIfNeed` emit; I checked the
edge cases (trailing lone backslash, `'a\\'`, `'a''` unterminated) and they
behave correctly.
--
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]
