github-actions[bot] commented on code in PR #65329:
URL: https://github.com/apache/doris/pull/65329#discussion_r3575978931
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -894,18 +1028,92 @@ 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);
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+
+ if (column.getType().isComplexType()) {
+ validateForModifyComplexColumn(column, currentCol,
columnPath.getFullPath());
+ applyComplexTypeChange(updateSchema, resolvedPath.getFullPath(),
currentCol.type(), column.getType());
Review Comment:
This nested complex `MODIFY COLUMN` path can change required Iceberg nested
fields to optional even when the user only intended a type/comment update.
`applyComplexTypeChange` later calls `makeColumnOptional` for required struct
fields, list elements, and map values whenever the replacement Doris type
reports nullable. But Doris cannot round-trip those required nested constraints
in a full complex type: parsed struct fields are always nullable, arrays are
treated as nullable, and map values default nullable. For an Iceberg table
created by another engine with a required nested field, `MODIFY COLUMN
info.struct_child STRUCT<...>` can therefore silently relax requiredness.
Please preserve existing Iceberg requiredness in full complex nested modifies,
or reject the operation when required nested constraints cannot be represented,
and add coverage for required struct/list/map nested fields.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -894,18 +1028,92 @@ 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);
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+
+ if (column.getType().isComplexType()) {
+ validateForModifyComplexColumn(column, currentCol,
columnPath.getFullPath());
+ applyComplexTypeChange(updateSchema, resolvedPath.getFullPath(),
currentCol.type(), column.getType());
+ if (column.isAllowNull()) {
+ updateSchema.makeColumnOptional(resolvedPath.getFullPath());
+ }
+ 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());
Review Comment:
This new nested modify path can still pass Doris types that Iceberg cannot
encode. `validateForModifyColumn` does not reject a primitive target like
`LARGEINT`, and this line then calls `dorisTypeToIcebergType`, whose primitive
mapper throws `UnsupportedOperationException` for `LARGEINT`. The nested add
path has the same gap, and the complex modify path can also reach it because
`ColumnType.checkSupportIcebergSchemaChangeForComplexType` currently allows
`INT/BIGINT -> LARGEINT` as a nested promotion even though Iceberg has no
corresponding type. Please add Iceberg-specific target-type validation before
nested add/modify conversion, keep complex Iceberg promotions limited to
encodable types, and cover nested add, direct modify, and full complex modify
to `LARGEINT` with validation tests.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/ColumnPath.java:
##########
@@ -0,0 +1,90 @@
+// 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.doris.analysis;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a column path used by schema change statements.
+ */
+public class ColumnPath {
+ private final ImmutableList<String> parts;
+
+ private ColumnPath(List<String> parts) {
+ Preconditions.checkArgument(parts != null && !parts.isEmpty(), "column
path is empty");
+ for (String part : parts) {
+ Preconditions.checkArgument(part != null && !part.isEmpty(),
"column path contains empty part");
+ }
+ this.parts = ImmutableList.copyOf(parts);
+ }
+
+ public static ColumnPath of(List<String> parts) {
+ return new ColumnPath(parts);
+ }
+
+ public static ColumnPath of(String name) {
+ return new ColumnPath(ImmutableList.of(name));
+ }
+
+ public static ColumnPath fromDotName(String name) {
+ return new ColumnPath(Arrays.asList(name.split("\\.")));
+ }
+
+ public List<String> getParts() {
+ return parts;
+ }
+
+ public boolean isNested() {
+ return parts.size() > 1;
+ }
+
+ public String getTopLevelName() {
+ return parts.get(0);
+ }
+
+ public String getLeafName() {
+ return parts.get(parts.size() - 1);
+ }
+
+ public ColumnPath getParentPath() {
+ Preconditions.checkState(isNested(), "top-level column path has no
parent");
+ return new ColumnPath(parts.subList(0, parts.size() - 1));
+ }
+
+ public String getParentPathString() {
+ return getParentPath().getFullPath();
+ }
+
+ public String getFullPath() {
+ return String.join(".", parts);
+ }
+
+ public String toSql() {
Review Comment:
`ColumnPath.toSql()` needs to escape the stored path parts, not just wrap
them in backticks. After the parser fixes from the existing quoted-identifier
comments, a valid path like ``info.`Metric``Name` `` will be stored as a
component containing a backtick, but this method renders it as invalid SQL
instead of doubling the embedded backtick. All of the new path-aware ALTER ops
use this renderer, and the same round-trip problem exists for `AFTER` because
`ColumnPosition.toSql()` wraps `lastCol` without escaping, and for rename
targets because `RenameColumnOp.toSql()` appends `newColName` unquoted. Please
use the normal identifier-quoting helper for all three renderers and add a
`toSql()`/round-trip test with escaped-backtick nested names, position
references, and rename targets.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -894,18 +1028,92 @@ 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);
+ UpdateSchema updateSchema = icebergTable.updateSchema();
+
+ if (column.getType().isComplexType()) {
+ validateForModifyComplexColumn(column, currentCol,
columnPath.getFullPath());
+ applyComplexTypeChange(updateSchema, resolvedPath.getFullPath(),
currentCol.type(), column.getType());
+ if (column.isAllowNull()) {
+ updateSchema.makeColumnOptional(resolvedPath.getFullPath());
+ }
+ 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());
Review Comment:
This nested primitive `MODIFY COLUMN` path accepts default metadata from the
parsed column definition but then drops it. `columnDefWithPath` allows
`DEFAULT` and `ON UPDATE`, and
`ColumnDefinition.translateToCatalogStyleForSchemaChange()` carries those
values into `column.getDefaultValue()`, `getDefaultValueExprDef()`, and
`hasOnUpdateDefaultValue()`. Nested ADD uses that default when calling Iceberg
`addColumn`, and complex MODIFY rejects defaults, but this branch only calls
`updateColumn(..., column.getComment())`. A statement like `MODIFY COLUMN s.a
BIGINT DEFAULT 7` can therefore succeed while the Iceberg default stays
unchanged/missing. Please either reject default/on-update clauses for nested
primitive MODIFY or apply the supported Iceberg default change explicitly, and
add coverage for both forms.
--
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]
