Copilot commented on code in PR #60169:
URL: https://github.com/apache/doris/pull/60169#discussion_r2719570518
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -716,15 +723,36 @@ public void renameColumn(ExternalTable dorisTable, String
oldName, String newNam
public void modifyColumn(ExternalTable dorisTable, Column column,
ColumnPosition position, long updateTime)
throws UserException {
Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
- validateForModifyColumn(column, icebergTable);
- Type icebergType =
IcebergUtils.dorisTypeToIcebergType(column.getType());
+ NestedField currentCol =
icebergTable.schema().findField(column.getName());
+ if (currentCol == null) {
+ throw new UserException("Column " + column.getName() + " does not
exist");
+ }
+
+ validateCommonColumnInfo(column);
UpdateSchema updateSchema = icebergTable.updateSchema();
- updateSchema.updateColumn(column.getName(),
icebergType.asPrimitiveType(), column.getComment());
- if (column.isAllowNull()) {
- // we can change a required column to optional, but not the other
way around
- // because we don't know whether there is existing data with null
values.
- updateSchema.makeColumnOptional(column.getName());
+
+ if (column.getType().isComplexType()) {
+ // Complex type processing branch
+ validateForModifyComplexColumn(column, currentCol);
+ applyComplexTypeChange(updateSchema, column.getName(),
currentCol.type(), column.getType());
+ if (column.isAllowNull()) {
+ updateSchema.makeColumnOptional(column.getName());
+ }
+ if (!Objects.equals(currentCol.doc(), column.getComment())) {
+ updateSchema.updateColumnDoc(column.getName(),
column.getComment());
+ }
+ } else {
Review Comment:
The new complex-type schema evolution logic in `modifyColumn`
(STRUCT/ARRAY/MAP handling, nested promotions, nullability checks, etc.) isn’t
covered by any tests in `IcebergMetadataOpTest` or other Iceberg tests, so
regressions here would be hard to catch. Given that the rest of the Iceberg
integration has unit tests, please consider adding tests that exercise
successful and failing `ALTER TABLE ... MODIFY COLUMN` cases for complex types
(e.g., struct field append, nested array element promotions, map value
promotions, illegal field renames).
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -737,23 +765,162 @@ public void modifyColumn(ExternalTable dorisTable,
Column column, ColumnPosition
refreshTable(dorisTable, updateTime);
}
- private void validateForModifyColumn(Column column, Table icebergTable)
throws UserException {
- validateCommonColumnInfo(column);
+ private void validateForModifyColumn(Column column, NestedField
currentCol) throws UserException {
// check complex type
if (column.getType().isComplexType()) {
throw new UserException("Modify column type to non-primitive type
is not supported: " + column.getType());
}
- // check exist
- NestedField currentCol =
icebergTable.schema().findField(column.getName());
- if (currentCol == null) {
- throw new UserException("Column " + column.getName() + " does not
exist");
- }
// check nullable
if (currentCol.isOptional() && !column.isAllowNull()) {
throw new UserException("Can not change nullable column " +
column.getName() + " to not null");
}
}
+ private void validateForModifyComplexColumn(Column column, NestedField
currentCol) throws UserException {
+ if (!column.getType().isComplexType()) {
+ throw new UserException("Modify column type to non-complex type is
not supported: " + column.getType());
+ }
+ org.apache.doris.catalog.Type oldDorisType =
IcebergUtils.icebergTypeToDorisType(
+ currentCol.type(), false, false);
+ org.apache.doris.catalog.Type newDorisType = column.getType();
+ try {
+ ColumnType.checkSupportSchemaChangeForComplexType(oldDorisType,
newDorisType, false);
+ } catch (DdlException e) {
+ throw new UserException(e.getMessage(), e);
+ }
+ if (currentCol.isOptional() && !column.isAllowNull()) {
+ throw new UserException("Cannot change nullable column " +
column.getName() + " to not null");
+ }
+ if (column.getDefaultValue() != null ||
column.getDefaultValueExprDef() != null) {
+ throw new UserException("Complex type default value only supports
NULL");
+ }
+ }
+
+ private void applyComplexTypeChange(UpdateSchema updateSchema, String path,
+ org.apache.iceberg.types.Type oldIcebergType,
+ org.apache.doris.catalog.Type newDorisType) throws UserException {
+ switch (oldIcebergType.typeId()) {
+ case STRUCT:
+ applyStructChange(updateSchema, path,
oldIcebergType.asStructType(), (StructType) newDorisType);
+ break;
+ case LIST:
+ applyListChange(updateSchema, path, (Types.ListType)
oldIcebergType, (ArrayType) newDorisType);
+ break;
+ case MAP:
Review Comment:
`applyComplexTypeChange` decides how to cast `newDorisType` based on
`oldIcebergType.typeId()`, but there is no validation that the new type has the
same complex kind (STRUCT/ARRAY/MAP) as the existing column. For example,
altering a STRUCT column to ARRAY (or a primitive column to a complex type)
will pass `validateForModifyComplexColumn` but then hit a `ClassCastException`
when casting `newDorisType` to `StructType`/`ArrayType`/`MapType`, resulting in
an internal error instead of a user-facing validation error. It would be safer
to explicitly check in `validateForModifyComplexColumn` (or here) that the old
and new Doris types are both complex and of the same kind, and throw a
`UserException` when they differ, before calling this method.
```suggestion
org.apache.doris.catalog.Type newDorisType) throws UserException
{
// Ensure the new Doris type is complex before performing any casts
based on the old Iceberg type.
if (!newDorisType.isComplexType()) {
throw new UserException("Modify column type to non-complex type
is not supported: " + newDorisType);
}
switch (oldIcebergType.typeId()) {
case STRUCT:
if (!(newDorisType instanceof StructType)) {
throw new UserException("Cannot modify STRUCT column to
type: " + newDorisType);
}
applyStructChange(updateSchema, path,
oldIcebergType.asStructType(), (StructType) newDorisType);
break;
case LIST:
if (!(newDorisType instanceof ArrayType)) {
throw new UserException("Cannot modify LIST/ARRAY column
to type: " + newDorisType);
}
applyListChange(updateSchema, path, (Types.ListType)
oldIcebergType, (ArrayType) newDorisType);
break;
case MAP:
if (!(newDorisType instanceof MapType)) {
throw new UserException("Cannot modify MAP column to
type: " + newDorisType);
}
```
--
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]
