jiangxt2 commented on code in PR #12918:
URL: https://github.com/apache/gravitino/pull/12918#discussion_r3990498099
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -1492,25 +1494,53 @@ private ShowCreateTableMetadata
parseCreateStatement(String createSql) {
return metadata;
}
- // 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);
Review Comment:
Thanks for the detailed review. Both `engine_full` samples from your comment
now parse correctly. Addressed in commit `e71a4518`.
`parseSettingsFromEngineFull()` now locates only the top-level `SETTINGS`
clause, skipping quoted text and balanced engine parameters. Based on
ClickHouse's `ASTStorage` formatting contract, `SETTINGS` is the final storage
clause, so the parser consumes the remainder of the string instead of relying
on a regex `COMMENT` terminator. This invariant follows ClickHouse's
[`ASTStorage::formatImpl`](https://github.com/ClickHouse/ClickHouse/blob/2497bf5356d42e9bd263af6dd4d20e59508b53b8/src/Parsers/ASTCreateQuery.cpp).
Malformed metadata before the `SETTINGS` clause now raises the same fixed
validation error instead of silently returning an empty settings map.
The added coverage includes:
- `COMMENT` inside a quoted setting value;
- `SETTINGS` inside quoted engine identifiers and engine parameters;
- unbalanced parentheses;
- unterminated quoted text;
- stray closing parentheses.
Spotless, the focused unit test, and `git diff --check` passed on the
current revision. The Docker-tagged ClickHouse IT has not been rerun for this
revision because the local macOS `docker-connector` is unavailable, so no
integration-test pass is being claimed.
--
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]
