jiangxt2 commented on code in PR #12918:
URL: https://github.com/apache/gravitino/pull/12918#discussion_r4052688039
##########
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:
Addressed in commit aa5dc48f: the parser now selects the last top-level
SETTINGS, with unit and native-load regression coverage.
I have not reproduced a top-level `COMMENT` suffix in `engine_full` on
ClickHouse 24.8.14.39; table comments appear in `create_table_query` instead. I
therefore left this guard out of the current PR. If a supported version
reproduces it, I can address it in a follow-up PR.
--
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]
