github-actions[bot] commented on code in PR #65815:
URL: https://github.com/apache/doris/pull/65815#discussion_r3642950390
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/operations/ExternalMetadataOps.java:
##########
@@ -263,6 +263,19 @@ default void renameColumn(ExternalTable dorisTable, String
oldName, String newNa
throw new UnsupportedOperationException("Rename column operation is
not supported for this table type.");
}
+ /**
+ * update properties for external table
+ *
+ * @param dorisTable external table
+ * @param properties properties to update
+ * @throws UserException if the update fails
+ */
+ default void updateTableProperties(ExternalTable dorisTable, Map<String,
String> properties)
+ throws UserException {
+ throw new UnsupportedOperationException(
Review Comment:
[P2] Keep unsupported connectors on the DDL error path
`ModifyTablePropertiesOp` now lets arbitrary properties through analysis for
every external catalog, so HMS/MaxCompute (which do not override this method)
reach this default. Throwing `UnsupportedOperationException` is then caught by
`StmtExecutor`'s generic “Maybe our bug” branch and returned as
`ERR_UNKNOWN_ERROR`, rather than a normal unsupported DDL error. Please either
capability-gate these catalogs during validation or throw/translate to
`DdlException`/`UserException` here so unsupported property changes stay on the
user-facing error path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -269,6 +273,30 @@ public void afterCreateTable(String dbName, String
tblName) {
dorisCatalog.getName(), dbName, tblName, db.isPresent());
}
+ @Override
+ public void renameTableImpl(String dbName, String oldName, String newName)
throws DdlException {
+ try {
+ executionAuthenticator.execute(() -> {
Review Comment:
[P1] Do not expose non-atomic object-store directory rename as safe DDL
For the filesystem catalog used by the new S3 regression, this delegates to
Paimon's directory `fileIO.rename`. Paimon 1.3.1's own `Catalog.renameTable`
contract warns that S3/OSS rename is non-atomic and a failure may move only
part of the table files; catching the exception here cannot roll that back.
Please reject filesystem-catalog rename on object storage (or provide a
recoverable/atomic metadata protocol) instead of treating the happy-path
copy/delete as supported table rename.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -269,6 +273,30 @@ public void afterCreateTable(String dbName, String
tblName) {
dorisCatalog.getName(), dbName, tblName, db.isPresent());
}
+ @Override
+ public void renameTableImpl(String dbName, String oldName, String newName)
throws DdlException {
+ try {
+ executionAuthenticator.execute(() -> {
+ catalog.renameTable(Identifier.create(dbName, oldName),
Identifier.create(dbName, newName), false);
Review Comment:
[P1] Rename the remote Paimon identifier, not the local alias
These arguments came from `ExternalTable.getDbName()/getName()`, which are
Doris-local names. Under supported lower-case modes, a remote `SalesDB.Orders`
is exposed as `salesdb.orders`; Paimon identifiers are case-sensitive, so this
call looks up the wrong source and returns table-not-found even though Doris
resolved the table. The other new Paimon ALTER paths correctly use
`getRemoteDbName()/getRemoteName()`. Please resolve/pass the remote source
identity for the catalog operation while retaining the local identity for cache
invalidation, and exercise the public rename path with non-identity names.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -392,6 +420,83 @@ public boolean databaseExist(String dbName) {
}
}
+ private SchemaChange addColumnChange(Column column, ColumnPosition
position) {
+ DataType paimonType =
toPaimontype(column.getType()).copy(column.isAllowNull());
Review Comment:
[P1] Support narrow integer columns on the new ADD path
`toPaimontype` throws `UnsupportedOperationException` for Doris `TINYINT`
and `SMALLINT`, even though Paimon supports both and the read path maps them
back to those Doris types. Because this conversion happens before `alterTable`
enters its catch-and-`UserException` boundary, a valid `ADD COLUMN c
TINYINT`/`SMALLINT` is reported as `ERR_UNKNOWN_ERROR`. Please add the missing
`TinyIntType`/`SmallIntType` mappings (including nested types) and keep
unsupported conversion failures on the normal DDL error path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -392,6 +420,83 @@ public boolean databaseExist(String dbName) {
}
}
+ private SchemaChange addColumnChange(Column column, ColumnPosition
position) {
+ DataType paimonType =
toPaimontype(column.getType()).copy(column.isAllowNull());
+ SchemaChange.Move move = null;
+ if (position != null) {
+ move = position.isFirst()
+ ? SchemaChange.Move.first(column.getName())
+ : SchemaChange.Move.after(column.getName(),
position.getLastCol());
Review Comment:
[P1] Resolve Paimon field names before building schema changes
Doris exposes every Paimon field as lowercase, while Paimon's schema-change
lookup is case-sensitive. For a remote field `Name`, `ADD ... AFTER name`,
`DROP COLUMN name`, and `RENAME COLUMN name` therefore target a nonexistent
field. The inverse also succeeds incorrectly: adding `Foo` beside remote `foo`
is allowed by Paimon, then Doris reloads both as duplicate `foo` columns.
Please resolve existing references to their remote field names and reject
case-insensitive collisions for new fields (including within multi-add), with
mixed-case coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -392,6 +420,83 @@ public boolean databaseExist(String dbName) {
}
}
+ private SchemaChange addColumnChange(Column column, ColumnPosition
position) {
+ DataType paimonType =
toPaimontype(column.getType()).copy(column.isAllowNull());
+ SchemaChange.Move move = null;
+ if (position != null) {
+ move = position.isFirst()
+ ? SchemaChange.Move.first(column.getName())
+ : SchemaChange.Move.after(column.getName(),
position.getLastCol());
+ }
+ return SchemaChange.addColumn(column.getName(), paimonType,
column.getComment(), move);
+ }
+
+ private void alterTable(ExternalTable dorisTable, List<SchemaChange>
changes, String operation)
+ throws UserException {
+ try {
+ executionAuthenticator.execute(() -> {
+ catalog.alterTable(
Review Comment:
[P1] Commit a multi-clause Paimon ALTER atomically
Each external ALTER clause reaches this helper separately, so a statement
such as `ADD COLUMN c INT, DROP COLUMN missing` commits `c` before the later
clause fails. Doris then reports the statement as failed while the remote
schema is already partially changed. Paimon already accepts an ordered
`List<SchemaChange>` in one schema-manager commit; please combine compatible
clauses into that single commit (or reject multi-clause ALTER before mutation)
and cover a later-clause failure.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonMetadataOps.java:
##########
@@ -392,6 +420,83 @@ public boolean databaseExist(String dbName) {
}
}
+ private SchemaChange addColumnChange(Column column, ColumnPosition
position) {
+ DataType paimonType =
toPaimontype(column.getType()).copy(column.isAllowNull());
+ SchemaChange.Move move = null;
+ if (position != null) {
+ move = position.isFirst()
+ ? SchemaChange.Move.first(column.getName())
+ : SchemaChange.Move.after(column.getName(),
position.getLastCol());
+ }
+ return SchemaChange.addColumn(column.getName(), paimonType,
column.getComment(), move);
Review Comment:
[P1] Preserve or reject added-column defaults
Doris accepts `ALTER TABLE p.t ADD COLUMN c INT DEFAULT 7` and the resulting
`Column` still carries `"7"`, but this conversion drops it. In the pinned
Paimon 1.3.1 API, `AddColumn` has no default field; a default is represented by
a separate `UpdateColumnDefaultValue`. The statement therefore succeeds while
creating a column without the requested default, so later writes that omit `c`
get different semantics. Please either emit the corresponding supported default
change or reject DEFAULT during analysis, and cover the resulting Paimon
`TableSchema` in the test.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
