github-actions[bot] commented on code in PR #66112:
URL: https://github.com/apache/doris/pull/66112#discussion_r3662852097
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateTableCommand.java:
##########
@@ -99,6 +99,10 @@ public void run(ConnectContext ctx, StmtExecutor executor)
throws Exception {
LOG.debug("Nereids start to execute the ctas command, query id:
{}, tableName: {}",
ctx.queryId(), createTableInfo.getTableName());
}
+ // Reject unsupported destinations before publishing metadata;
rollback by table name
+ // cannot distinguish this CTAS table from a concurrent replacement
with the same name.
+ query =
UnboundTableSinkCreator.createUnboundTableSink(createTableInfo.getTableNameParts(),
Review Comment:
[P1] Preserve the existing-table `IF NOT EXISTS` no-op
This sink check now runs before `createTable()`, so `CREATE TABLE IF NOT
EXISTS ... AS SELECT` against an already-existing Paimon table throws `Load
data to PaimonExternalCatalog is not supported` instead of taking the catalog's
no-op path. `PaimonMetadataOps.performCreateTable()` would return `true` for
that stable existing table, and the command would return at lines 107-108
without ever writing. This is separate from the rollback race in the existing
thread: no metadata is created or dropped here; a valid idempotent DDL simply
regresses to an error. Please preserve the atomic existing/created result while
rejecting unsupported sinks only for a newly created target, and add an
existing-table Paimon CTAS `IF NOT EXISTS` regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/RequestPropertyDeriver.java:
##########
@@ -202,11 +202,9 @@ public Void visitPhysicalIcebergDeleteSink(
@Override
public Void visitPhysicalIcebergMergeSink(
PhysicalIcebergMergeSink<? extends Plan> icebergMergeSink,
PlanContext context) {
- if (connectContext != null &&
!connectContext.getSessionVariable().enableStrictConsistencyDml) {
- addRequestPropertyToChildren(PhysicalProperties.ANY);
- } else {
-
addRequestPropertyToChildren(icebergMergeSink.getRequirePhysicalProperties());
- }
+ // MERGE cardinality is a statement invariant: matching row IDs must
be co-located even
+ // when the optional strict-consistency routing for other DML
statements is disabled.
+
addRequestPropertyToChildren(icebergMergeSink.getRequirePhysicalProperties());
Review Comment:
[P2] Keep relaxed routing for plain Iceberg UPDATE
`PhysicalIcebergMergeSink` is also the sink produced by
`IcebergUpdateCommand`, not only by SQL MERGE. Removing the session-variable
branch therefore makes `enable_strict_consistency_dml=false` force a row-id
hash or `MERGE_PARTITIONED` exchange for every plain UPDATE, even though that
command scans each target row once and has no multiple-source cardinality
ambiguity. Large UPDATEs now pay the network repartitioning cost that the
switch previously avoided. The new test covers only `MergeIntoCommand`, so this
parallel producer is invisible. Please carry whether SQL MERGE cardinality
enforcement is required into the logical/physical sink (or split the sink
types), override the switch only for that path, and add a
strict-consistency-off UPDATE plan test.
##########
regression-test/suites/external_table_p0/PAIMON_ICEBERG_READ_WRITE_P0_COVERAGE.md:
##########
@@ -75,7 +75,7 @@ Paimon ecosystem matrix are boundary tests rather than
positive Doris P0 contrac
| Paimon | Deletion vectors, upsert/delete visibility and data/system tables |
Covered | `test_paimon_deletion_vector`, `paimon_data_system_table`,
`paimon_system_table` |
| Paimon | Catalog/database/table create and drop | Covered |
`test_create_paimon_table` |
| Paimon | Doris data write-back | Negative boundary covered |
`test_paimon_write_boundary` |
-| Paimon | Failed CTAS metadata atomicity | Isolated known-bug regression |
`test_paimon_ctas_atomicity_negative` |
+| Paimon | Failed CTAS metadata atomicity | Active negative regression |
`test_paimon_ctas_atomicity_negative` |
Review Comment:
[P2] Update the PM04 audit narrative with its activation
This row now says PM04 is active, but the same document still calls it an
opt-in regression for a remaining defect at lines 46-47 and says the current
product leaves the table at line 98. Since this patch removes the gate and
makes the no-leftover contract the active regression behavior, those
PM04-specific passages now contradict the suite and status row. Please update
the audit text and PM04 design row together with this activation.
##########
regression-test/suites/external_table_p0/iceberg/write/ICEBERG_WRITE_P0_COVERAGE.md:
##########
@@ -70,10 +70,10 @@ under the License.
| Doris 源 bucket | HASH 固定 bucket、RANDOM bucket、HASH AUTO bucket | 已覆盖（验证通过） |
`test_iceberg_write_source_models` |
| 分区源类型 | STRING/INT/BIGINT/DATE/DATETIME/DECIMAL 的 bucket 与适用
transform；BOOLEAN identity 与非法 bucket | 已覆盖（验证通过） |
`test_iceberg_write_partition_types_null` |
| NULL 分区 | identity NULL、数值/decimal bucket 与 truncate NULL、time transform
NULL、多列组合 NULL | 已覆盖（验证通过） | `test_iceberg_write_partition_types_null` |
-| nullable STRING truncate | nullable STRING 经过 truncate transform 的 INSERT，以及
UPDATE 产生的 Nullable projection 写入 | 已覆盖（隔离负向） |
`test_iceberg_write_nullable_truncate_negative` |
+| nullable STRING truncate | nullable STRING 经过 truncate transform 的 INSERT，以及
UPDATE 产生的 Nullable projection 写入 | 已覆盖（常规回归） |
`test_iceberg_write_nullable_truncate_negative` |
Review Comment:
[P2] Align the activated suites' execution narratives
This row (together with lines 75-76) now classifies W08, W10, and W19 as
regular regressions, and this patch removes their crash/known-bug gates. The
detailed matrix still calls all three `隔离负向` at lines 103, 105, and 114, still
requires a restartable isolated cluster for W08/W19, and the conclusion at line
118 says such reproductions retain explicit isolation switches. The W19 suite
itself also says at lines 68-70 that the writer must reject its nullable MERGE
projection as invalid input, although the active test now expects the MERGE and
physical truncate-partition checks to succeed. Please update the three design
rows, the W19 suite comment, and the conclusion to describe ordinary P0
execution/current fixed behavior, while retaining isolation language only for
suites such as the required-null tests that still have a gate.
##########
be/src/exec/sink/viceberg_merge_sink.cpp:
##########
@@ -120,17 +124,26 @@ Status VIcebergMergeSink::write(RuntimeState* state,
Block& block) {
if (delete_op) {
delete_filter[i] = 1;
has_delete = true;
- ++_delete_row_count;
++delete_rows;
}
if (insert_op) {
insert_filter[i] = 1;
has_insert = true;
- ++_insert_row_count;
++insert_rows;
}
}
+ // The physical sink hashes matched rows by row_id, so identical targets
reach the same sink.
+ // Exact state retained across blocks therefore enforces MERGE cardinality
for the whole query.
+ RETURN_IF_ERROR(_validate_matched_row_ids(output_block,
delete_filter.data()));
+ if (_matched_row_id_state_bytes_counter != nullptr) {
+ COUNTER_SET(_matched_row_id_state_bytes_counter,
+ static_cast<int64_t>(_matched_row_id_state_bytes()));
Review Comment:
[P1] Avoid rescanning all retained files for every block
Every successful `write()` calls `_matched_row_id_state_bytes()`, which
walks the complete accumulated file-to-bitmap map and asks every bitmap for its
size. If `F_b` files are retained after block `b`, this counter alone performs
`sum_b F_b` cache-unfriendly visits and can reach `Theta(F * B)` when many of
`F` files remain live across `B` blocks; when files and blocks co-grow, that
can approach quadratic work even though validation itself is incremental. The
new 100,000-row test uses one file in one block, so it cannot expose this path.
This is distinct from the existing retained-memory thread: the issue is the new
counter's repeated traversal. Please maintain the byte count from only the
entries touched by the current block, or compute it at a non-hot lifecycle
boundary, and cover many distinct files across multiple writes.
--
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]
