yuqi1129 commented on code in PR #12918:
URL: https://github.com/apache/gravitino/pull/12918#discussion_r3988603346
##########
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:
[P2] Make SETTINGS clause extraction quote-aware before enforcing structural
validation.
`parseSettingsFromEngineFull()` still uses `SETTINGS_PATTERN`, which matches
`SETTINGS` and terminates at `COMMENT` even inside quoted text. The new
validation therefore rejects valid server metadata. For example, ClickHouse
24.8.14 accepts:
```sql
CREATE TABLE quoted_comment (id Int32)
ENGINE = MergeTree ORDER BY id
SETTINGS merge_workload = 'gravitino,COMMENT,comma';
```
Its `engine_full` is:
```text
MergeTree ORDER BY id SETTINGS merge_workload = 'gravitino,COMMENT,comma',
index_granularity = 8192
```
The regex passes only `merge_workload = 'gravitino,` to this method, so this
check throws `IllegalArgumentException: Invalid ClickHouse table SETTINGS
metadata`. That propagates through `getSystemTableMetadata()` and aborts
`loadTable()`.
The start boundary has the same issue: a valid `ReplacingMergeTree` table
with a UInt64 version column named `SETTINGS version` produces:
```text
ReplacingMergeTree(`SETTINGS version`) ORDER BY id SETTINGS
index_granularity = 8192
```
The regex starts inside the quoted identifier, and parsing fails at this
same line.
I reproduced both native table definitions with the project's ClickHouse
24.8.14 image, then passed the returned metadata to the actual PR parser in two
supplemental regression tests; both fail here. The existing 103 unit tests
pass. The regex bug predates this change, but the new strict validation turns
incorrect property readback into a table-load failure.
Please locate the clause boundaries with quote/parenthesis-aware scanning as
well, and add these cases to the regression coverage.
--
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]
