yuqi1129 commented on code in PR #13498:
URL: https://github.com/apache/gravitino/pull/13498#discussion_r4120796498
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -953,6 +983,80 @@ SystemTableMetadata getSystemTableMetadata(
throw new NoSuchTableException("Table %s does not exist in %s.",
tableName, databaseName);
}
+ @VisibleForTesting
+ @Nullable
+ String getProjectionProperty(Connection connection, String databaseName,
String tableName)
+ throws SQLException {
+ // Probe the stable columns directly so ClickHouse 24.8, which has no
system.projections table,
+ // can continue serving ordinary catalog metadata. Only UNKNOWN_TABLE
means the capability is
+ // unavailable; access and other query errors must remain visible to the
caller.
+ try (PreparedStatement probe =
+ connection.prepareStatement("SELECT database FROM
system.projections LIMIT 0");
+ ResultSet probeResult = probe.executeQuery()) {
+ if (probeResult.next()) {
+ throw new SQLException(
+ "Unexpected row returned by the system.projections capability
probe");
+ }
+ } catch (SQLException e) {
+ if (e.getErrorCode() == ERROR_CODE_UNKNOWN_TABLE) {
+ if (projectionSourceUnavailableWarningLogged.compareAndSet(false,
true)) {
+ LOG.warn(
+ "ClickHouse does not provide system.projections; projection
metadata round-trip "
+ + "requires ClickHouse 24.9 or later");
+ }
+ return null;
+ }
+ throw e;
+ }
+
+ Set<String> projectionColumns = new HashSet<>();
+ try (PreparedStatement statement =
+ connection.prepareStatement(
+ "SELECT name FROM system.columns "
+ + "WHERE database = 'system' AND table = 'projections'");
+ ResultSet resultSet = statement.executeQuery()) {
+ while (resultSet.next()) {
+ projectionColumns.add(resultSet.getString("name"));
+ }
+ }
+ if (!projectionColumns.containsAll(REQUIRED_PROJECTION_COLUMNS)) {
+ Set<String> missingColumns = new HashSet<>(REQUIRED_PROJECTION_COLUMNS);
+ missingColumns.removeAll(projectionColumns);
+ throw new SQLException(
+ "system.projections is missing required columns: "
+ + missingColumns.stream().sorted().collect(Collectors.joining(",
")));
+ }
+
+ boolean hasSettingsColumn = projectionColumns.contains("settings");
+ String query =
+ "SELECT name, type, query"
+ + (hasSettingsColumn ? ", toJSONString(settings) AS settings_json"
: "")
+ + " FROM system.projections WHERE database = ? AND table = ? ORDER
BY name";
+ List<ClickHouseTableSqlUtils.ProjectionDefinition> definitions = new
ArrayList<>();
+ try (PreparedStatement statement = connection.prepareStatement(query)) {
+ statement.setString(1, databaseName);
+ statement.setString(2, tableName);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ while (resultSet.next()) {
+ Map<String, String> settings =
+ hasSettingsColumn
+ ? ClickHouseTableSqlUtils.parseProjectionSettings(
+ resultSet.getString("settings_json"))
+ : Collections.emptyMap();
+ definitions.add(
+ new ClickHouseTableSqlUtils.ProjectionDefinition(
+ resultSet.getString("name"),
+ resultSet.getString("type"),
+ resultSet.getString("query"),
+ settings));
+ }
+ }
+ }
+ return definitions.isEmpty()
+ ? null
+ : ClickHouseTableSqlUtils.serializeProjectionDefinitions(definitions);
Review Comment:
A projection that the server accepts but this connector does not support
makes the whole table unloadable.
`serializeProjectionDefinitions` runs `validateProjectionDefinition` on
every row read from `system.projections`. It throws `IllegalArgumentException`
for `_part_offset`, a top-level `WHERE`, an unknown type, or a setting value
outside the safe-literal rules. `load()` only catches `SQLException`, so the
exception escapes `loadTable`.
Reproduced on ClickHouse 25.12.2.54, the version used by
`CatalogClickHouseProjectionIT`:
```sql
CREATE TABLE t (a UInt64, b String, PROJECTION p (SELECT _part_offset ORDER
BY b))
ENGINE = MergeTree ORDER BY a;
-- system.projections: name = p, type = Normal, query = SELECT _part_offset
ORDER BY b
```
With this PR, `loadTable(t)` fails with `IllegalArgumentException`, although
it loads fine today. `alterTable` fails too, because it loads the table through
`getOrCreateTable` before it generates the ALTER SQL. So the table becomes
unusable through Gravitino instead of just missing projection metadata.
Suggestion: keep the strict validation for user input on `createTable`, but
on load skip the property when the server returns a definition the connector
cannot round-trip:
```java
try {
return ClickHouseTableSqlUtils.serializeProjectionDefinitions(definitions);
} catch (IllegalArgumentException e) {
LOG.warn("Skip projection metadata of {}.{}: {}", databaseName, tableName,
e.getMessage());
return null;
}
```
The `parseProjectionSettings(settings_json)` call above needs the same
handling. I'd omit the whole property rather than drop only the unsupported
entries: a partial list would silently lose a projection on recreate.
Could you also add:
- a unit test where `system.projections` returns `SELECT _part_offset ORDER
BY b`, asserting `getProjectionProperty` returns `null` and `load()` still
succeeds with the other properties;
- a table with a `_part_offset` projection in the 25.12 IT case, asserting
`loadTable` succeeds without `clickhouse.projections`?
Since the load path is affected, please rerun the 24.8 targeted IT and the
cluster suite on the final head as well.
--
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]
