jerryshao commented on code in PR #13588:
URL: https://github.com/apache/gravitino/pull/13588#discussion_r4132487896
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -93,6 +96,61 @@ public JdbcTablePartitionOperations
createJdbcTablePartitionOperations(JdbcTable
dataSource, loadedTable, exceptionMapper, typeConverter);
}
+ /** {@inheritDoc} */
+ @Override
+ public void create(
+ String databaseName,
+ String tableName,
+ JdbcColumn[] columns,
+ @Nullable String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ @Nullable SortOrder[] sortOrders) {
+ LOG.info("Attempting to create table {} in database {}", tableName,
databaseName);
+ try (Connection connection = getConnection(databaseName)) {
+ String sql =
+ generateCreateTableSql(
+ tableName,
+ columns,
+ comment,
+ properties,
+ partitioning,
+ distribution,
+ indexes,
+ sortOrders);
+ JdbcConnectorUtils.executeUpdate(connection, sql);
+
+ // Doris 2.1.0's Nereids CREATE TABLE path can discard the table
comment. Repair it only
+ // when necessary, without changing the planner on the pooled
connection. Keep the full
+ // comment, including the Gravitino identifier, so subsequent loads
retain table identity.
+ try {
+ if (StringUtils.isNotEmpty(comment)
+ && !comment.equals(loadTableComment(connection, databaseName,
tableName))) {
Review Comment:
[Question] Was an intentional decision made to run this verification on
every Doris version rather than only the affected ones? Two consequences worth
confirming:
1. `StringUtils.isNotEmpty(comment)` never short-circuits in practice -
`JdbcCatalogOperations` always passes
`StringIdentifier.addToComment(identifier, comment)`
(`JdbcCatalogOperations.java:501-505`), and `addToComment` returns the
identifier text even for a null/blank comment
(`core/.../StringIdentifier.java:166-173`). So every `CREATE TABLE`, on 1.2.x,
3.0.x and 4.0.x alike, now pays one extra `information_schema.TABLES` round
trip.
2. On those unaffected versions a failure of that lookup (or of the repair)
turns a create that previously succeeded into an error with the table left
behind, and per the new doc section the JDBC user now needs ALTER on freshly
created tables.
The class already has version detection that could gate this -
`getDorisVersion` / `isVersionAtLeast` as used by
`validateAutoIncrementVersion` (lines 322-332) - though it opens its own
connection per call, so gating has its own cost. Either way is defensible; a
comment recording the trade-off would help future readers.
Verified by: reading `JdbcCatalogOperations.createTable` (lines 461-510),
`StringIdentifier.addToComment`, and
`validateAutoIncrementVersion`/`getDorisVersion` in this repo at this commit.
##########
catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris2xIT.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.catalog.doris.integration.test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.Collections;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.StringIdentifier;
+import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
+import org.apache.gravitino.integration.test.container.DorisContainer;
+import org.apache.gravitino.integration.test.container.DorisImageName;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.Table;
+import org.apache.gravitino.rel.TableCatalog;
+import org.apache.gravitino.rel.expressions.NamedReference;
+import org.apache.gravitino.rel.expressions.distributions.Distributions;
+import org.apache.gravitino.rel.expressions.transforms.Transforms;
+import org.apache.gravitino.rel.types.Types;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+/** Integration tests for Doris 2.1.0 with the Nereids planner enabled. */
+public class CatalogDoris2xIT extends CatalogDorisIT {
Review Comment:
[Important] Extending `CatalogDorisIT` pulls its 23 inherited `@Test`
methods into the 2.1.0 run, not just the new one. `--tests '*CatalogDoris2xIT'`
therefore executes 25 test cases against the
`apache/doris:doris-all-in-one-2.1.0` container, which does not match the "3
integration tests" reported in the PR description, and some inherited
assertions are explicitly written for 1.2.x semantics - e.g.
`CatalogDorisIT.java:263-298` (`testTablePropertiesRoundTrip`) documents 1.2.x
`SHOW CREATE TABLE` behaviour and asserts `compression` "should appear ... on
Doris 1.2.x".
The existing convention for version-specific suites is a standalone class:
`CatalogDoris3xIT.java:89` and `CatalogDoris4xIT.java:90` both extend `BaseIT`
and re-declare only the version-relevant tests. Suggest following that
convention (or stating that the full inherited suite was run green on 2.1.0),
also because `build.gradle.kts:517-527` gates this tag on
`-PdorisMultiVersionTest` precisely because extra Doris containers exhaust the
runner.
Verified by: counting `@Test` in `CatalogDorisIT` (23) at this commit,
reading its `testTablePropertiesRoundTrip` assertions, the class declarations
of `CatalogDoris3xIT`/`CatalogDoris4xIT`, and the `doris-multi-version`
exclusion in `build.gradle.kts`.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -93,6 +96,61 @@ public JdbcTablePartitionOperations
createJdbcTablePartitionOperations(JdbcTable
dataSource, loadedTable, exceptionMapper, typeConverter);
}
+ /** {@inheritDoc} */
+ @Override
+ public void create(
+ String databaseName,
+ String tableName,
+ JdbcColumn[] columns,
+ @Nullable String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ @Nullable SortOrder[] sortOrders) {
+ LOG.info("Attempting to create table {} in database {}", tableName,
databaseName);
+ try (Connection connection = getConnection(databaseName)) {
Review Comment:
[Nit] This override copies the body of `JdbcTableOperations.create(...,
sortOrders)` (`JdbcTableOperations.java:131-159`) verbatim - same log lines,
same `generateCreateTableSql` call, same `executeUpdate`, same exception
mapping - and only appends the repair block. Any future change to the base
implementation would silently bypass Doris.
Since the repair only needs a connection to `databaseName` and not the
CREATE's own connection, `super.create(...)` followed by the repair in its own
`try (Connection connection = getConnection(databaseName))` would keep the base
behaviour inherited. The unit tests would still pass, as they stub
`generateCreateTableSql` and `DataSource.getConnection()` rather than the base
method.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -93,6 +96,61 @@ public JdbcTablePartitionOperations
createJdbcTablePartitionOperations(JdbcTable
dataSource, loadedTable, exceptionMapper, typeConverter);
}
+ /** {@inheritDoc} */
+ @Override
+ public void create(
+ String databaseName,
+ String tableName,
+ JdbcColumn[] columns,
+ @Nullable String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ @Nullable SortOrder[] sortOrders) {
+ LOG.info("Attempting to create table {} in database {}", tableName,
databaseName);
+ try (Connection connection = getConnection(databaseName)) {
+ String sql =
+ generateCreateTableSql(
+ tableName,
+ columns,
+ comment,
+ properties,
+ partitioning,
+ distribution,
+ indexes,
+ sortOrders);
+ JdbcConnectorUtils.executeUpdate(connection, sql);
+
+ // Doris 2.1.0's Nereids CREATE TABLE path can discard the table
comment. Repair it only
+ // when necessary, without changing the planner on the pooled
connection. Keep the full
+ // comment, including the Gravitino identifier, so subsequent loads
retain table identity.
+ try {
+ if (StringUtils.isNotEmpty(comment)
+ && !comment.equals(loadTableComment(connection, databaseName,
tableName))) {
+ JdbcConnectorUtils.executeUpdate(
+ connection,
+ "ALTER TABLE `"
+ + tableName
+ + "` MODIFY COMMENT \""
+ + escapeSqlLiteral(comment, '"')
+ + "\"");
+ }
+ } catch (SQLException e) {
Review Comment:
[Important] The inner `catch` only covers `SQLException`, so the missing-row
case escapes `create()` unmapped. `loadTableComment` throws
`NoSuchTableException` (`DorisTableOperations.java:1325`), which is unchecked
and not a `SQLException`, so it passes through both this catch and the outer
one at line 149 and surfaces from `createTable` as `Table db.t does not exist
in Doris when loading its comment`. At that point `CREATE TABLE` has already
committed, so the user gets none of the guidance this block was written to
provide (table remains, may lack the Gravitino identifier, must be dropped
before retrying), and `TableCatalog.createTable` does not declare
`NoSuchTableException`.
Suggest catching it here too (or having `loadTableComment` return
`Optional`/a sentinel and raising the same `GravitinoRuntimeException` from the
create path), so every post-CREATE failure reports the same actionable message.
In the load path the behaviour is fine: `JdbcTableOperations.load` already
throws `NoSuchTableException` from `getTableBuilder`
(`JdbcTableOperations.java:223`, called from `load` at line 253) before
`correctJdbcTableFields` runs.
Verified by: reading `create()` and `loadTableComment` in full at this
commit, plus
`TestDorisTableCreation.testMissingTableIsNotTreatedAsEmptyComment`, which
asserts exactly this propagation (`NoSuchTableException` out of `create`), and
`JdbcTableOperations.load` lines 250-292.
--
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]
