github-actions[bot] commented on code in PR #66620:
URL: https://github.com/apache/doris/pull/66620#discussion_r3749578635
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalIcebergMergeSink.java:
##########
@@ -73,9 +73,9 @@ public LogicalIcebergMergeSink(IcebergExternalDatabase
database,
this.targetIcebergTable = Objects.requireNonNull(
targetIcebergTable, "targetIcebergTable != null in
LogicalIcebergMergeSink");
// Delete-only MERGE writes position deletes and never invokes the
unsupported data writer.
- // UPDATE and data-producing MERGE must still reject an unchanged
Variant target column.
+ // UPDATE and data-producing MERGE must use the same Variant
capability checks as INSERT.
if (writesDataFiles) {
- IcebergUtils.validateWriteSchema(cols);
+ IcebergUtils.validateWriteSchema(targetIcebergTable, cols);
Review Comment:
[P1] Do not classify user `operation` columns as merge metadata
This enables data-producing merge writes, but the downstream merge binder
identifies metadata solely by output name. For a legal target column named
`operation`, UPDATE/MERGE projects both the routing alias and the user column
with that name; both are excluded from `dataExprCount`, so the count mismatch
returns before target casts and Variant checks. BE removes only the first
`operation` slot as routing metadata and writes the second as table data,
allowing a legacy Variant assignment to reach the new struct-backed Iceberg
Variant writer and fail during execution. Please identify routing fields by
position or expression identity, validate the aligned data outputs, and add
quoted-`operation` UPDATE/MERGE coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -726,12 +727,25 @@ public static boolean containsVariant(Type type) {
return false;
}
- public static void validateWriteSchema(List<Column> columns) {
- if (columns.stream().anyMatch(column ->
containsVariant(column.getType()))) {
- // Keep this table capability read-only until every Iceberg writer
can preserve the
- // Variant physical identity; rejecting only selected columns
would allow data loss.
+ public static void validateWriteSchema(Table table, List<Column> columns) {
+ if (columns.stream().noneMatch(column ->
containsVariant(column.getType()))) {
+ return;
+ }
+ validateWriteSchema(columns, getFormatVersion(table),
getFileFormat(table));
Review Comment:
[P1] Gate Variant writes during smooth backend upgrades
This enables every v3/Parquet Variant write without the
backend-compatibility check already used for Variant reads. An `INSERT ...
VALUES` or internal-source write has no `IcebergScanNode`, and smooth-upgrade
source BEs remain query/load eligible for sink placement. FE serializes the
target schema with the Iceberg `variant` primitive, but a pre-patch BE writer
lacks the new `from_primitive_string("variant")` branch and fails while opening
the sink. Please reject or schedule table and merge writer fragments only on
compatible BEs during rolling upgrades, and add mixed-version INSERT plus
data-producing UPDATE/MERGE coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -1407,6 +1421,24 @@ public static long getIcebergRowCount(ExternalTable tbl)
{
public static FileFormat getFileFormat(Table icebergTable) {
Map<String, String> properties = icebergTable.properties();
String fileFormatName = resolveFileFormatName(properties);
+ return parseFileFormatName(fileFormatName);
+ }
+
+ public static FileFormat getEffectiveFileFormat(Map<String, String>
tableProperties,
+ Map<String, String> catalogProperties) {
+ String fileFormatName =
catalogProperties.get(CatalogProperties.TABLE_OVERRIDE_PREFIX
Review Comment:
[P2] Use the runtime file-format precedence during CREATE validation
This gives a catalog override of `write.format.default` precedence over all
table properties, but `getFileFormat(Table)` later calls
`configuredFileFormatName` and gives the persisted `write-format` alias
precedence over `write.format.default`. Since the catalog applies the override
under the standard key without removing the differently named alias,
conflicting settings can make CREATE reject a table that Doris would write as
Parquet, or accept one whose later INSERT/UPDATE checks resolve to ORC and
reject. Please use one canonical precedence rule for pre-create and
loaded-table resolution, and test both alias/override permutations.
--
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]
