[fix](be) Release deletion vector rows on read errors#65351
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
05e82db to
18b2449
Compare
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65351 Problem Summary: Deletion vector and position-delete cache builders allocated DeleteRows with raw new and returned nullptr on read or parse failures. Those error paths leaked the partially built DeleteRows object and left the failure behavior under-tested in the V1 Iceberg and Paimon readers. Use RAII while building DeleteRows and release ownership only after successful construction. Add UT coverage for V1 Iceberg reader deletion vector read failures, V1 Paimon Parquet/ORC deletion vector read failures, the shared Iceberg helper fault paths, and the format v2 TableReader deletion vector fault paths. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - Added BE unit tests for V1/shared Iceberg deletion vector helper fault and stress paths. - Added BE unit tests for V1 Iceberg reader deletion vector read failure cache-builder path. - Added BE unit tests for V1 Paimon Parquet/ORC reader deletion vector read failure cache-builder paths. - Added BE unit tests for format v2 Iceberg deletion vector fault paths. - Extended Iceberg deletion vector regression to cover both scanner v1 and scanner v2 debug point fault injection. - Ran build-support/clang-format.sh locally. - Ran PATH=/usr/local/opt/llvm@16/bin:$PATH build-support/check-format.sh locally. - Ran git diff --check locally. - On gabriel@10.26.20.3 in /mnt/disk3/gabriel/Workspace/dev1/doris_pr65351_ut, ran ./run-be-ut.sh --run --filter="IcebergReaderTest.v1_deletion_vector_read_error_releases_cache_entry:PaimonDeletionVectorTest.V1*:IcebergDeleteFileReaderHelperTest.*:IcebergV2ReaderTest.IcebergDeletionVector*:IcebergV2ReaderTest.IcebergTableReader*DeletionVector*"; 24 tests passed. - Attempted build-support/run-clang-tidy.sh --build-dir be/cmake-build-debug-dev-perf locally with CLANG_TIDY_BINARY=/usr/local/opt/llvm@16/bin/clang-tidy; blocked by the local macOS bash 3.2 missing mapfile. - Behavior changed: No - Does this need documentation: No
18b2449 to
a597245
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review completed for PR 65351 at head a597245.
I found one ownership issue that should be fixed before merge. The PR converts the deletion-vector DeleteRows cache builders to RAII, but the V1 Iceberg position-delete DeleteFile cache builder in the same method still leaks when _read_position_delete_file fails.
Critical checkpoint conclusions:
- Goal/test coverage: The deletion-vector read-error leak paths are covered by new unit/regression tests, but the parallel V1 Iceberg position-delete read-error path remains uncovered and still has the same cache-builder ownership problem.
- Scope/focus: The main code change is small and focused; the missed issue is a directly adjacent parallel path in touched code.
- Concurrency/lifecycle:
KVCache::getserializes creation under its shard lock; no new concurrency issue found. The remaining issue is lifecycle/ownership on a builder failure path. - Config/session/compatibility: No production config or serialization compatibility change found. The regression toggles
enable_file_scanner_v2and restores it infinally. - Parallel paths: V1 Iceberg DV, V1 Paimon DV, and format-v2 DV builders use
unique_ptrcorrectly; V1 Iceberg position-deleteDeleteFilebuilder is the missed parallel path. - Tests/results: I did not run BE/regression tests in this runner because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing. Static review only. - Observability, persistence, data writes: Not materially affected by this PR.
User focus: no additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite reported OR-001 as a duplicate of MAIN-001 and found no additional optimizer/rewrite or reader-semantics issue. tests-session-config reported TSC-001 as duplicate/supporting evidence for MAIN-001 and found no additional tests/session/config issue. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger and proposed final comment set.
| @@ -660,10 +666,10 @@ Status IcebergReaderMixin<BaseReader>::_position_delete_base( | |||
| SCOPED_TIMER(_iceberg_profile.delete_rows_sort_time); | |||
| _iceberg_delete_rows = | |||
| _kv_cache->template get<DeleteRows>(data_file_path, [&]() -> DeleteRows* { | |||
There was a problem hiding this comment.
This RAII conversion fixes the DeleteRows builder here, but _position_delete_base still has the same leak pattern in the DeleteFile cache builder just above it: auto* position_delete = new DeleteFile, then _read_position_delete_file(...), and on any non-OK status it returns nullptr. KVCache::get only takes ownership when the builder returns a non-null pointer, so a missing/corrupt position-delete file still leaks the partially built DeleteFile. Please wrap that builder in std::unique_ptr<DeleteFile> too and release only after _read_position_delete_file succeeds.
### What problem does this PR solve? Issue Number: None Related PR: apache#65351 Problem Summary: Deletion vector and position-delete cache builders allocated rows/maps with raw `new` and returned `nullptr` on read or parse failures. Those error paths leaked partially built cache values because `KVCache::get` only takes ownership when the builder returns a non-null pointer. Use RAII while building `DeleteRows` and `DeleteFile`, and release ownership only after successful construction. Add fault-injection/regression coverage for V1 Iceberg deletion-vector and position-delete failures, V1 Paimon Parquet/ORC deletion-vector failures, the shared Iceberg helper, and the format v2 TableReader path. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - Added BE unit tests for V1/shared Iceberg deletion vector helper fault and stress paths. - Added BE unit tests for V1 Iceberg reader deletion vector and position-delete read failure cache-builder paths. - Added BE unit tests for V1 Paimon Parquet/ORC reader deletion vector read failure cache-builder paths. - Added BE unit tests for format v2 Iceberg deletion vector fault paths. - Extended Iceberg deletion vector regression to cover both scanner v1 and scanner v2 debug point fault injection. - Ran build-support/clang-format.sh locally. - Ran PATH=/usr/local/opt/llvm@16/bin:$PATH build-support/check-format.sh locally. - Ran git diff --check locally. - On gabriel@10.26.20.3 in /mnt/disk3/gabriel/Workspace/dev1/doris_pr65351_ut, ran ./run-be-ut.sh --run --filter="IcebergReaderTest.v1_position_delete_read_error_releases_cache_entry:IcebergReaderTest.v1_deletion_vector_read_error_releases_cache_entry:PaimonDeletionVectorTest.V1*:IcebergDeleteFileReaderHelperTest.*:IcebergV2ReaderTest.IcebergDeletionVector*:IcebergV2ReaderTest.IcebergTableReader*DeletionVector*"; 25 tests passed. - Attempted build-support/run-clang-tidy.sh --build-dir be/cmake-build-debug-dev-perf locally with CLANG_TIDY_BINARY=/usr/local/opt/llvm@16/bin/clang-tidy; blocked by the local macOS bash 3.2 missing mapfile. - Behavior changed: No - Does this need documentation: No
a597245 to
658e7ee
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Part 1.3 checkpoint conclusions:
- Goal/test proof: the RAII ownership changes address the deletion-vector/position-delete cache leak goal, and negative coverage was added, but one new V1 regression assertion is invalid and is called out inline.
- Scope/focus: the code changes are focused on cache-builder ownership and fault-injection coverage.
- Concurrency/lifecycle: no new shared concurrency or static/global lifecycle risk was found; the main lifecycle change improves ownership by using
unique_ptruntil cache publication succeeds. - Config/compatibility/parallel paths: no new persistent format, thrift, or rolling-upgrade compatibility concern was found. V1/V2 Iceberg and Paimon paths were checked; the V1
should_stoptest path needs correction. - Error handling/data correctness/performance/observability: error propagation and cache insertion behavior are mostly preserved; no additional observability appears necessary for this leak fix.
- Tests/results: BE unit and regression coverage were added, but the V1
should_stopregression should be fixed before relying on it.
Subagent conclusions: optimizer-rewrite and tests-session-config proposed no candidates in their initial pass. After MAIN-1 and the proposed final comment set were recorded, final convergence round 1 ended with both subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger state.
User focus: no additional user-provided review focus was supplied.
Validation: static review only in this runner. .worktree_initialized and thirdparty/installed were missing, so I did not run local builds or tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#65351 Problem Summary: Deletion vector and position-delete cache builders allocated rows/maps with raw `new` and returned `nullptr` on read or parse failures. Those error paths leaked partially built cache values because `KVCache::get` only takes ownership when the builder returns a non-null pointer. Use RAII while building `DeleteRows` and `DeleteFile`, and release ownership only after successful construction. Add fault-injection/regression coverage for V1 Iceberg deletion-vector and position-delete failures, V1 Paimon Parquet/ORC deletion-vector failures, the shared Iceberg helper, and the format v2 TableReader path. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - Added BE unit tests for V1/shared Iceberg deletion vector helper fault and stress paths. - Added BE unit tests for V1 Iceberg reader deletion vector and position-delete read failure cache-builder paths. - Added BE unit tests for V1 Paimon Parquet/ORC reader deletion vector read failure cache-builder paths. - Added BE unit tests for format v2 Iceberg deletion vector fault paths. - Extended Iceberg deletion vector regression to cover both scanner v1 and scanner v2 debug point fault injection. - Ran build-support/clang-format.sh locally. - Ran PATH=/usr/local/opt/llvm@16/bin:$PATH build-support/check-format.sh locally. - Ran git diff --check locally. - On gabriel@10.26.20.3 in /mnt/disk3/gabriel/Workspace/dev1/doris_pr65351_ut, ran ./run-be-ut.sh --run --filter="IcebergReaderTest.v1_position_delete_read_error_releases_cache_entry:IcebergReaderTest.v1_deletion_vector_read_error_releases_cache_entry:PaimonDeletionVectorTest.V1*:IcebergDeleteFileReaderHelperTest.*:IcebergV2ReaderTest.IcebergDeletionVector*:IcebergV2ReaderTest.IcebergTableReader*DeletionVector*"; 25 tests passed. - Attempted build-support/run-clang-tidy.sh --build-dir be/cmake-build-debug-dev-perf locally with CLANG_TIDY_BINARY=/usr/local/opt/llvm@16/bin/clang-tidy; blocked by the local macOS bash 3.2 missing mapfile. - Behavior changed: No - Does this need documentation: No
658e7ee to
bc11dbe
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review completed for PR 65351 at head bc11dbe.
I found one remaining test issue that should be fixed before relying on this regression coverage: the new scanner-v2 should_stop case expects a SQL exception for an END_OF_FILE status that the scanner framework treats as normal EOS.
Critical checkpoint conclusions:
- Goal/test proof: the RAII cache-builder changes address the intended leak pattern across the touched V1/V2 Iceberg and Paimon paths, with added unit/regression coverage. One new regression assertion is invalid and is called out inline.
- Scope/focus: the production change is small and focused on cache-builder ownership and fault injection.
- Concurrency/lifecycle:
KVCache::getstill serializes cache creation under the shard lock; the ownership changes improve failure-path lifecycle by releasing only after successful construction. - Config/session/compatibility: no production config, serialization, storage-format, or rolling-upgrade compatibility change was found. The regression toggles
enable_file_scanner_v2and restores it infinally. - Parallel paths: V1 Iceberg DV/position-delete, V1 Paimon DV, and format-v2 DV paths were checked. The older V1
should_stopthread is addressed in current head, but the current V2should_stopSQL assertion has the same EOF-as-EOS problem. - Error handling/data correctness/performance/observability: error propagation and cache insertion behavior are otherwise preserved; no additional observability appears necessary for this leak fix.
- Tests/results:
git diff --checkwas clean for the PR files. I did not run BE/regression tests because.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing in this runner.build-support/check-format.shcould not run because the runner has clang-format 18 instead of the required v16.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite found no new valuable candidate beyond existing threads. tests-session-config proposed TSC-1; I accepted it as M-1 and prepared the inline comment. Final convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger and proposed final comment set.
### What problem does this PR solve? Issue Number: None Related PR: apache#65351 Problem Summary: Deletion vector and position-delete cache builders allocated rows/maps with raw `new` and returned `nullptr` on read or parse failures. Those error paths leaked partially built cache values because `KVCache::get` only takes ownership when the builder returns a non-null pointer. Use RAII while building `DeleteRows` and `DeleteFile`, and release ownership only after successful construction. Add fault-injection/regression coverage for V1 Iceberg deletion-vector and position-delete failures, V1 Paimon Parquet/ORC deletion-vector failures, the shared Iceberg helper, and the format v2 TableReader path. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - Added BE unit tests for V1/shared Iceberg deletion vector helper fault and stress paths. - Added BE unit tests for V1 Iceberg reader deletion vector and position-delete read failure cache-builder paths. - Added BE unit tests for V1 Paimon Parquet/ORC reader deletion vector read failure cache-builder paths. - Added BE unit tests for format v2 Iceberg deletion vector fault paths. - Extended Iceberg deletion vector regression to cover both scanner v1 and scanner v2 debug point fault injection. - Ran build-support/clang-format.sh locally. - Ran PATH=/usr/local/opt/llvm@16/bin:$PATH build-support/check-format.sh locally. - Ran git diff --check locally. - On gabriel@10.26.20.3 in /mnt/disk3/gabriel/Workspace/dev1/doris_pr65351_ut, ran ./run-be-ut.sh --run --filter="IcebergReaderTest.v1_position_delete_read_error_releases_cache_entry:IcebergReaderTest.v1_deletion_vector_read_error_releases_cache_entry:PaimonDeletionVectorTest.V1*:IcebergDeleteFileReaderHelperTest.*:IcebergV2ReaderTest.IcebergDeletionVector*:IcebergV2ReaderTest.IcebergTableReader*DeletionVector*"; 25 tests passed. - Attempted build-support/run-clang-tidy.sh --build-dir be/cmake-build-debug-dev-perf locally with CLANG_TIDY_BINARY=/usr/local/opt/llvm@16/bin/clang-tidy; blocked by the local macOS bash 3.2 missing mapfile. - Behavior changed: No - Does this need documentation: No
bc11dbe to
6e94309
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed for PR 65351 at head 6e94309.
I did not find any new substantiated blocking issue in the current head. I am not adding inline comments: the three live prior concerns are already present in existing threads, and the current patch appears to address them by converting the remaining position-delete builder to RAII and by replacing the regression EOF should_stop expectations with non-EOF injected errors for scanner V1 and V2.
Part 1.3 checkpoint conclusions:
- Goal/test proof: The PR goal is to avoid leaking partially built delete-vector/position-delete cache values on read/parse errors. Current head uses
std::unique_ptrwhile buildingDeleteRows/DeleteFileand releases only on successful cache insertion paths. The patch adds BE unit coverage for Iceberg/Paimon V1, Iceberg helper, and format-v2 TableReader fault paths, plus regression coverage for scanner V1/V2 injected read failures. - Scope/focus: The production changes are narrow and limited to cache-builder ownership, shared Iceberg DV decode reuse, and debug-point injection for test coverage.
- Concurrency/lifecycle:
KVCache::getstill serializes creation under the shard mutex and only owns non-null returned pointers. The RAII locals are destroyed on failed create callbacks and released only on success. No new thread or long-lived shared lifecycle is introduced. - Configuration/session: No new config item is added. The regression temporarily changes
enable_file_scanner_v2and restores it infinally; BE debug points are also cleared infinally. - Compatibility/storage/protocol: No storage format, persisted metadata, thrift protocol, or mixed-version contract changes were found.
- Parallel paths: V1 Iceberg, V1 Paimon Parquet/ORC, shared Iceberg helper, and format-v2 Iceberg DV paths were reviewed. Scanner V1 and scanner V2 regression paths are both covered by non-EOF injected errors in the current head.
- Testing: Added tests target the relevant error paths. I could not run them in this runner because the checkout is missing
.worktree_initialized,thirdparty/installed, and the base/parent commit needed for local diff checks; validation here is static plus GitHub PR patch/API inspection. - Observability/performance: No new observability need was identified. The ownership changes do not add material hot-path work beyond RAII wrapper construction.
- User focus: No additional user-provided review focus was present.
Subagent conclusions:
optimizer-rewrite: no optimizer/rewrite or parallel join/aggregate candidate found; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: no test/session/config candidate found beyond existing threads; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.
Final opinion: comment-only review with no new inline comments for the current head.
TPC-H: Total hot run time: 29641 ms |
TPC-DS: Total hot run time: 180177 ms |
ClickBench: Total hot run time: 25.11 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run external |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Deletion vector and position-delete cache builders allocated rows/maps with raw
newand returnednullptron read or parse failures. Those error paths leaked partially built cache values becauseKVCache::getonly takes ownership when the builder returns a non-null pointer. This PR uses RAII while buildingDeleteRowsandDeleteFile, releases ownership only after successful construction, and adds fault-injection/regression coverage for V1 Iceberg deletion-vector and position-delete failures, V1 Paimon Parquet/ORC deletion-vector failures, the shared Iceberg helper, and the format v2 TableReader path.Release note
None
Check List (For Author)
build-support/clang-format.shlocally.PATH=/usr/local/opt/llvm@16/bin:$PATH build-support/check-format.shlocally.git diff --checklocally.gabriel@10.26.20.3in/mnt/disk3/gabriel/Workspace/dev1/doris_pr65351_ut, ran./run-be-ut.sh --run --filter="IcebergReaderTest.v1_position_delete_read_error_releases_cache_entry:IcebergReaderTest.v1_deletion_vector_read_error_releases_cache_entry:PaimonDeletionVectorTest.V1*:IcebergDeleteFileReaderHelperTest.*:IcebergV2ReaderTest.IcebergDeletionVector*:IcebergV2ReaderTest.IcebergTableReader*DeletionVector*"; 25 tests passed.build-support/run-clang-tidy.sh --build-dir be/cmake-build-debug-dev-perflocally withCLANG_TIDY_BINARY=/usr/local/opt/llvm@16/bin/clang-tidy; blocked by the local macOS bash 3.2 missingmapfile.