github-actions[bot] commented on code in PR #65329:
URL: https://github.com/apache/doris/pull/65329#discussion_r3577433517
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -894,18 +1028,94 @@ public void modifyColumn(ExternalTable dorisTable,
Column column, ColumnPosition
refreshTable(dorisTable, updateTime);
}
+ @Override
+ public void modifyColumn(ExternalTable dorisTable, ColumnPath columnPath,
Column column, ColumnPosition position,
+ long updateTime) throws UserException {
+ if (!columnPath.isNested()) {
+ modifyColumn(dorisTable, column, position, updateTime);
+ return;
+ }
+
+ Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
+ ResolvedColumnPath resolvedPath =
resolveColumnPath(icebergTable.schema(), columnPath, "modify");
+ NestedField currentCol = resolvedPath.getField();
+
+ validateCommonColumnInfo(column);
+ if (column.hasDefaultValue() || column.hasOnUpdateDefaultValue()) {
+ throw new UserException("Modifying default values is not supported
for Iceberg columns: " + columnPath);
+ }
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+
+ if (column.getType().isComplexType()) {
+ validateForModifyComplexColumn(column, currentCol,
columnPath.getFullPath());
+ applyComplexTypeChange(updateSchema, resolvedPath.getFullPath(),
currentCol.type(), column.getType());
+ // Column does not preserve whether NULL was explicit or omitted
for MODIFY.
+ // Keep the existing target requiredness instead of silently
relaxing it.
+ if (!Objects.equals(currentCol.doc(), column.getComment())) {
+ updateSchema.updateColumnDoc(resolvedPath.getFullPath(),
column.getComment());
+ }
+ } else {
+ validateForModifyColumn(column, currentCol,
columnPath.getFullPath());
+ Type icebergType =
IcebergUtils.dorisTypeToIcebergType(column.getType());
+ updateSchema.updateColumn(resolvedPath.getFullPath(),
icebergType.asPrimitiveType(), column.getComment());
+ if (column.isAllowNull()) {
+ updateSchema.makeColumnOptional(resolvedPath.getFullPath());
Review Comment:
This primitive nested `MODIFY COLUMN` branch still relaxes required Iceberg
fields when nullability is omitted. For `MODIFY COLUMN info.metric BIGINT`, the
parser leaves nullability as `DEFAULT`; `ColumnNullableType.DEFAULT` becomes
nullable for ordinary primitives, and
`translateToCatalogStyleForSchemaChange()` copies that into
`column.isAllowNull()`. This branch then calls
`makeColumnOptional(resolvedPath.getFullPath())`, so a required nested
primitive field from a Spark-created schema can become optional even though the
statement only changed the type/comment. The complex branch just above
preserves existing requiredness because `Column` cannot tell omitted `NULL`
from explicit `NULL`; please apply the same rule here unless the user
explicitly requested a supported nullability relaxation, and add coverage with
a required primitive nested field.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -848,42 +956,68 @@ public void renameColumn(ExternalTable dorisTable, String
oldName, String newNam
refreshTable(dorisTable, updateTime);
}
+ @Override
+ public void renameColumn(ExternalTable dorisTable, ColumnPath columnPath,
String newName, long updateTime)
+ throws UserException {
+ if (!columnPath.isNested()) {
+ renameColumn(dorisTable, columnPath.getTopLevelName(), newName,
updateTime);
+ return;
+ }
+ Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
+ ResolvedColumnPath resolvedPath =
validateNestedStructFieldPath(icebergTable.schema(), columnPath, "rename");
+ ResolvedColumnPath parentPath =
resolveColumnPath(icebergTable.schema(), columnPath.getParentPath(), "rename");
+
validateNoCaseInsensitiveSiblingCollision(parentPath.getType().asStructType(),
+ parentPath.getColumnPath(), newName, resolvedPath.getField(),
"rename");
+
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+ updateSchema.renameColumn(resolvedPath.getFullPath(), newName);
+ try {
+ executionAuthenticator.execute(() -> updateSchema.commit());
+ } catch (Exception e) {
+ throw new UserException("Failed to rename nested column: " +
columnPath.getFullPath() + " to " + newName
+ + " in table: " + icebergTable.name() + ", error message
is: " + e.getMessage(), e);
+ }
+ refreshTable(dorisTable, updateTime);
+ }
+
@Override
public void modifyColumn(ExternalTable dorisTable, Column column,
ColumnPosition position, long updateTime)
throws UserException {
Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
- NestedField currentCol =
icebergTable.schema().findField(column.getName());
+ NestedField currentCol =
icebergTable.schema().caseInsensitiveFindField(column.getName());
Review Comment:
This new case-insensitive top-level lookup makes mixed-case required Iceberg
fields reachable through Doris, but the existing modify logic can then silently
relax them to optional. Doris exposes Iceberg top-level names lower-cased and
nullable, so a Spark schema with required `Id` is seen as `id`; `ALTER TABLE t
MODIFY COLUMN id BIGINT` leaves nullability as `DEFAULT`, which becomes
`column.isAllowNull() == true`. After this line resolves `Id`, the primitive
and complex branches still call `makeColumnOptional(columnPath.getFullPath())`,
so a type/comment-only modify can change required `Id` to optional. Please
preserve the existing Iceberg requiredness when nullability was omitted, or
carry explicit `NULL` intent separately, and add coverage for required
mixed-case top-level primitive and complex fields.
--
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]
