morningman opened a new pull request, #69291:
URL: https://github.com/apache/doris/pull/69291
### What problem does this PR solve?
Issue Number: None
Related PR: #69010, #62344
Problem Summary:
**In short.** This ports the test of branch-4.1 #62344 to master and adds
the tests that master's broader fix, #69010, does not have: an `INSERT
OVERWRITE` whose source the planner proves empty must still replace its target
with nothing. Master behaves correctly since #69010, but none of its tests
reaches the code path #69010 changed, so taking that change back out keeps
every test green. The tests added here fail without it.
**Background.** When the planner folds the source of an `INSERT OVERWRITE`
to an empty relation (`LIMIT 0`, `WHERE false`, a contradiction) and the sink
needs no exchange below it, the sink sits directly on `PhysicalEmptyRelation`.
`InsertIntoTableCommand` then treats the insert as empty, and no backend runs
it. An empty `INSERT` has nothing to commit, but an empty overwrite still has
to replace the table, or the static partition, with nothing. Before #69010,
`requiresTransaction()` was false for every empty insert, so the overwrite
never reached the connector: the statement returned OK and every row stayed.
branch-4.1 #62344 fixed this for an Iceberg static partition overwrite only
(`IcebergInsertExecutor.shouldExecuteEmptyInsert`); its master pick #65105
changes `IcebergInsertExecutor`, which master no longer has. #69010 made
`PluginDrivenInsertExecutor.requiresTransaction()` true for every connector
overwrite, which covers #62344's case and the others. Both connectors already c
ommit an overwrite that receives no data: Paimon commits
`withOverwrite(staticPartition)` with no messages, and Iceberg overwrites the
matching files with no new ones.
**The problem, and what it cost.** No test checks any of this.
`PluginDrivenInsertExecutorTest` has no case for `requiresTransaction()`, and
the empty overwrites in the regression suites never put the sink on the empty
relation, so they pass with or without #69010's change:
| Existing case | Plan on master | Without #69010's `requiresTransaction`
override |
|---|---|---|
| `test_iceberg_partition_overwrite_mode`, `static_optimized_empty`
(partitioned table, `WHERE false`, strict DML off) | hash exchange for the
partition routing, the writers run | clears the table anyway |
| `test_paimon_write_transaction`, `txn_empty_overwrite_limit_zero`
(append-only table, `LIMIT 0`) | `ExecutionAny` exchange, the writers run |
clears the table anyway |
On master the sink sits directly on the empty relation for a Paimon
primary-key table without a fixed bucket count, which is Paimon's default and
is written by a single writer, and for an unpartitioned Iceberg table when
`enable_strict_consistency_dml` is false, the default in cloud mode. On a local
cluster with an FE built without the override, that is, master before #69010:
| Statement | Without the override | master |
|---|---|---|
| `INSERT OVERWRITE TABLE t SELECT 1, 1 LIMIT 0`, Paimon primary-key table
without `bucket` | returns OK, both rows stay, no snapshot | table cleared,
OVERWRITE snapshot |
| `INSERT OVERWRITE TABLE t PARTITION (p = 'a') SELECT 1, 1 LIMIT 0`, the
same kind of table, partitioned | returns OK, both partitions stay | only
partition `a` cleared |
| `INSERT OVERWRITE TABLE t SELECT * FROM t LIMIT 0`, unpartitioned Iceberg
table, strict DML off | returns OK, all three rows stay | table cleared |
So a change that brought the shortcut back, or a connector that returned
early when it receives no data, would silently skip such overwrites again, and
no test would notice. branch-4.2 still skips the first and the third (only an
Iceberg static partition overwrite runs its transaction there); that is
reported to its maintainers separately.
**How this PR fixes it.** Tests only:
- `PluginDrivenInsertExecutorTest`: an empty insert that overwrites the
whole table or a static partition requires its transaction; an empty `INSERT`
does not.
- `PaimonConnectorTransactionTest`: an overwrite that receives no commit
data replaces its static partition with nothing and keeps the other partition,
and a whole-table overwrite clears the table, both with an OVERWRITE snapshot.
The table is a real on-disk Paimon table written through the Paimon API.
- `IcebergConnectorTransactionTest`: an Iceberg static partition overwrite
with no data deletes only the file of the matching partition, ported from the
branch-4.1 test of #62344.
- `test_paimon_write_transaction`: `LIMIT 0` overwrites of a primary-key
table without `bucket` and of a static partition of one. `EXPLAIN SHAPE PLAN`
pins the sink on `PhysicalEmptyRelation`; the suite then checks the rows, the
snapshots, and what Spark reads. The comment of the existing append-only `LIMIT
0` case said "A direct PhysicalEmptyRelation"; it now says that the table is
written through an exchange.
- `test_iceberg_insert_overwrite_with_empty_table`: a `LIMIT 0` overwrite of
an unpartitioned table with strict DML consistency off, with its plan.
**Results.**
- FE UT: `PluginDrivenInsertExecutorTest` passes (`run-fe-ut.sh`, 11 cases).
Removing the override turns the new overwrite case red; requiring a transaction
for every empty insert turns the new INSERT case red. The new Paimon cases fail
when the connector also returns early for an overwrite with no commit data, and
the Iceberg case fails when a static partition overwrite with no data returns
early. All 826 tests of fe-connector-paimon and all 1611 of
fe-connector-iceberg pass.
- Regression: both suites pass on a local cluster against the iceberg docker
environment, and fail with the override removed (at
`txn_dynamic_bucket_empty_overwrite` and `q4`). The regenerated `.out` files
differ from master's only by the new blocks.
Original author: @suxiaogang223 (the Iceberg static partition overwrite
test, from #62344)
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- `test_paimon_write_transaction` and
`test_iceberg_insert_overwrite_with_empty_table` pass, and fail with #69010's
override removed.
- [x] Unit Test
- New cases in `PluginDrivenInsertExecutorTest`,
`PaimonConnectorTransactionTest` and `IcebergConnectorTransactionTest`; all
fe-connector-paimon and fe-connector-iceberg tests pass.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
--
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]
