github-actions[bot] commented on code in PR #66620:
URL: https://github.com/apache/doris/pull/66620#discussion_r3751124080
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java:
##########
@@ -297,31 +298,54 @@ private LogicalIcebergMergeSink<Plan>
bindIcebergMergeSink(
List<NamedExpression> outputExprs = sink.child().getOutput().stream()
.map(NamedExpression.class::cast)
.collect(ImmutableList.toImmutableList());
- List<Column> visibleColumns = sink.getCols().stream()
- .filter(Column::isVisible)
- .collect(ImmutableList.toImmutableList());
- int dataExprCount = 0;
- for (NamedExpression expr : outputExprs) {
- if (!isIcebergMergeMetaColumn(expr.getName())) {
- dataExprCount++;
- }
- }
- if (dataExprCount != visibleColumns.size()) {
+ List<NamedExpression> castExprs = coerceIcebergMergeOutput(
+ sink.getCols(), outputExprs, sink.isWritesDataFiles());
+ if (castExprs.equals(outputExprs)) {
if (sink.getOutputExprs().equals(outputExprs)) {
return sink;
}
return sink.withOutputExprs(outputExprs);
}
+ LogicalProject<?> project = new LogicalProject<>(castExprs,
sink.child());
+ return (LogicalIcebergMergeSink<Plan>)
sink.withChildAndUpdateOutput(project);
+ }
+
+ static List<NamedExpression> coerceIcebergMergeOutput(List<Column>
sinkColumns,
+ List<NamedExpression> outputExprs, boolean writesDataFiles) {
+ List<Column> outputColumns = sinkColumns.stream()
+ .filter(column -> column.isVisible() ||
IcebergUtils.isIcebergRowLineageColumn(column))
+ .collect(ImmutableList.toImmutableList());
+ int expectedOutputCount = 2 + outputColumns.size();
+ if (outputExprs.size() != expectedOutputCount
+ ||
!IcebergMergeOperation.OPERATION_COLUMN.equalsIgnoreCase(outputExprs.get(0).getName())
+ ||
!Column.ICEBERG_ROWID_COL.equalsIgnoreCase(outputExprs.get(1).getName())) {
+ throw new AnalysisException("Iceberg merge sink output is not
aligned with its routing metadata "
+ + "and target columns");
+ }
+
+ List<Column> visibleColumns =
Lists.newArrayListWithCapacity(outputColumns.size());
+ List<NamedExpression> visibleOutputExprs =
Lists.newArrayListWithCapacity(outputColumns.size());
+ for (int i = 0; i < outputColumns.size(); ++i) {
+ Column column = outputColumns.get(i);
+ if (column.isVisible()) {
+ visibleColumns.add(column);
+ visibleOutputExprs.add(outputExprs.get(i + 2));
+ }
+ }
+ if (writesDataFiles) {
Review Comment:
[P1] Coerce MERGE actions before combining their branches
`visibleOutputExprs` has already been produced by
`generateFinalProjections`, which folds every action into nested `If`s. Those
`If`s choose a common branch type, and Variant plus a primitive chooses the
primitive type. For example, if the matched action leaves an object Variant
unchanged while the not-matched action inserts integer `1`, the object is first
cast to DECIMAL (the BE scalar cast maps objects to SQL NULL), so this
validation sees only DECIMAL and the later target cast leaves the value SQL
NULL. Please validate and coerce each action projection against its target
column before building the branch `If`s, and cover an unchanged object Variant
plus a primitive insert action.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -781,6 +810,7 @@ public void addColumns(ExternalTable dorisTable,
List<Column> columns, long upda
Table icebergTable = IcebergUtils.getIcebergTable(dorisTable);
for (Column column : columns) {
validateAddColumnMetadata(column, true);
+ validateVariantSchema(icebergTable, column.getType(),
column.getName(), false);
validateRowLineageColumnMutation(icebergTable, column.getName(),
"add");
}
Review Comment:
[P2] Handle existing Variant columns in MODIFY COLUMN
This admits a top-level Variant on a v3/Parquet table, but the next
`validateForModifyColumn` call requires `currentCol.type().isPrimitiveType()`.
Iceberg Variant is primitive-like without implementing `Type.PrimitiveType`, so
a valid same-type MODIFY (for example making a required Variant optional or
moving it with `FIRST`/`AFTER`) is rejected as complex-to-primitive; later code
also calls `asPrimitiveType()`, including on a requested Variant target. Please
give existing Variant-to-Variant operations a dedicated path, explicitly reject
conversions to or from Variant, and add CREATE/ADD followed by MODIFY coverage.
--
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]
