[fix](iow) force drop partition in INSERT OVERWRITE#62510
Merged
Conversation
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29256 ms |
Contributor
TPC-DS: Total hot run time: 171233 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: request changes. The PR goal is to ensure INSERT OVERWRITE no longer leaves replaced partitions in the recycle bin, but the change only affects the non-auto-detect path. Auto-detect insert overwrite (PARTITION(*)) still uses InsertOverwriteManager.taskGroupSuccess(), which calls InsertOverwriteUtil.replacePartition(targetTable, oldNames, newNames) without force, so replaced partitions remain recoverable and continue loading the recycle bin.
Critical checkpoint conclusions:
- Goal/test: The stated goal is not fully accomplished; no replacement test was added for the new non-recoverable behavior, and all existing recover-related tests were deleted.
- Scope/clarity: The code change is small, but too narrow because it misses a parallel commit path.
- Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle, config, or serialization compatibility risk found in the edited line.
- Parallel paths: Blocking issue found in the auto-detect task-group path.
- Tests: Coverage is insufficient; deleted tests should be replaced with tests that assert recovery fails after INSERT OVERWRITE for both normal and auto-detect paths.
- Observability/performance/transaction/persistence: No additional issue found beyond the incorrect force-drop propagation.
- User focus: No additional user-provided review focus was supplied.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking issue. The code path now propagates forceDropPartition through both direct and auto-detect insert-overwrite replacement paths, so the previously raised auto-detect implementation gap appears addressed. However, the PR removes the recycle-bin recovery regression tests without adding replacement coverage for the new behavior.
Critical checkpoint conclusions:
- Goal/test proof: the implementation aims to keep INSERT OVERWRITE replaced partitions out of the recycle bin, but the PR does not include a regression test proving recover now fails or is impossible for direct, whole-table, unpartitioned, and auto-detect paths.
- Scope: the code change is small and focused, but deleting the existing tests leaves the behavior unguarded.
- Concurrency/lifecycle/config: no new concurrency, special lifecycle, or config concerns found beyond existing insert-overwrite manager behavior.
- Compatibility/protocol: the added optional Thrift field is wire-compatible; no additional FE-BE variable path found.
- Parallel paths: the prior direct vs auto-detect path gap is now handled in code, but needs coverage.
- Test coverage/results: blocking gap; deleted tests are not replaced with negative/positive assertions for the new recycle-bin semantics.
- Observability/transactions/persistence/performance: no additional issue found in the changed lines.
User focus: no additional user-provided review focus was supplied.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29896 ms |
Contributor
TPC-DS: Total hot run time: 172117 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
starocean999
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
"Insert overwrite" can be achieved by using "replace partition". By default, the replaced partition will be placed in the recycle bin. However, in the "insert overwrite" scenario, the partitions placed in the recycle bin make it difficult to restore the table to its previous state. And if a large number of partitions are placed in the recycle bin, it will significantly increase the load of the recycle bin. Therefore, now the "insert overwrite" operation no longer places the replaced partitions into the recycle bin.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)