github-actions[bot] commented on code in PR #69010:
URL: https://github.com/apache/doris/pull/69010#discussion_r4256299795
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java:
##########
@@ -707,6 +720,8 @@ private void buildPendingOperation() {
case OVERWRITE:
if (staticPartitionOverwrite) {
commitStaticPartitionOverwrite(buildDataWriteResults());
+ } else if (wholeTableOverwrite) {
+ commitWholeTableOverwrite(buildDataWriteResults());
Review Comment:
[P1] Commit static overwrite for a planner-proven empty source. On a
populated partitioned Iceberg table, `SET enable_strict_consistency_dml=false;
SET iceberg_partition_overwrite_mode='static'; INSERT OVERWRITE TABLE t SELECT
* FROM t WHERE FALSE;` leaves all old rows intact.
EliminateFilter/EliminateEmptyRelation reduce `ConnectorSink -> Project ->
Filter(FALSE) -> Scan` to `ConnectorSink -> PhysicalEmptyRelation`:
RequestPropertyDeriver relaxes the parallel-write sink to ANY, so no
distribution enforcer separates the sink from Empty. InsertIntoTableCommand
sets emptyInsert, and AbstractInsertExecutor.requiresTransaction() returns
false, skipping beginWrite/finalizeSink and returning before this overwrite
branch runs. The cloud getter disables strict consistency by default, so this
also affects default cloud execution. Require the static overwrite transaction
to begin/finalize/commit for empty sources, and add a WHERE FALSE or LIMIT 0
regression with strict consistency disabled.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3653,6 +3653,20 @@ public void
checkAnnIndexCandidateRowsPercentThreshold(String value) {
description = "Enable merge partitioning for Iceberg UPDATE/DELETE
(INSERT by partition columns, "
+ "DELETE by row_id).")
public boolean enableIcebergMergePartitioning = true;
+
+ public static final String ICEBERG_PARTITION_OVERWRITE_MODE =
"iceberg_partition_overwrite_mode";
+ public static final String ICEBERG_PARTITION_OVERWRITE_MODE_DYNAMIC =
"dynamic";
+ public static final String ICEBERG_PARTITION_OVERWRITE_MODE_STATIC =
"static";
+ @VarAttrDef.VarAttr(name = ICEBERG_PARTITION_OVERWRITE_MODE, needForward =
true,
+ description = "Scope of INSERT OVERWRITE on an Iceberg table when
no PARTITION clause is given. "
+ + "'dynamic' (default, Hive-compatible and identical to
Spark's "
+ + "spark.sql.sources.partitionOverwriteMode=dynamic): only
the partitions the new data "
+ + "lands in are replaced, so data written under an older
partition spec is left untouched. "
+ + "'static' (identical to Spark's default
partitionOverwriteMode=static): the whole table "
+ + "is overwritten, including data written under an older
partition spec.",
+ options = {ICEBERG_PARTITION_OVERWRITE_MODE_DYNAMIC,
ICEBERG_PARTITION_OVERWRITE_MODE_STATIC})
Review Comment:
[P2] Validate the overwrite mode when it is assigned. VarAttr.options is
descriptive metadata; VariableMgr.setValue assigns String fields directly
unless a checker/setter is registered, and this variable has neither. SET
iceberg_partition_overwrite_mode='statc' (or an empty/unsupported value)
therefore succeeds, and beginWrite treats it as dynamic, silently retaining
unwritten partitions. Add a case-insensitive dynamic/static checker to the
session variable so SET and SET_VAR reject unsupported values, while preserving
the connector's fallback for an absent property.
##########
regression-test/data/external_table_p0/iceberg/write/test_iceberg_partition_overwrite_mode.out:
##########
@@ -0,0 +1,21 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !before --
+1 1
+1 2
+1 3
+
+-- !dynamic_empty --
+1 1
+1 2
+1 3
+
+-- !static_empty --
+
+-- !static_data --
+7 3
+
+-- !dynamic_data --
+1 1
+9 2
Review Comment:
[P2] Regenerate the golden in order_qt sort order. order_qt_dynamic_data
sorts returned rows by all columns through Suite.quickRunTest and
DataUtils.sortByToString, even though SQL uses ORDER BY val. The actual
comparison order is (1,1), (7,3), (9,2), while this block expects (1,1), (9,2),
(7,3); OutputUtils.checkOutput does not sort expected rows. The new regression
therefore fails at dynamic_data even when overwrite behavior is correct.
Regenerate this output through the regression script, or use qt_dynamic_data
with the explicit SQL ordering and regenerate.
--
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]
