[bug](iceberg) fix iceberg sink writer with spill report error#62899
Merged
Conversation
Contributor
Author
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking issue in the concurrency fix. The PR correctly tries to serialize FullSorter operations, but the spill path still reads VIcebergTableWriter::_current_writer without synchronization while the async writer thread assigns to the same shared_ptr, so the race can happen before VIcebergSortWriter::_sorter_mutex is reached.
Critical checkpoint conclusions:
- Goal/test: The goal is to fix Iceberg sort writer spill errors under memory revocation. The current code only partially accomplishes it; no PR test or reproduction coverage was added.
- Scope: The code change is small and focused, but the synchronization boundary is incomplete.
- Concurrency: Yes. The async writer thread performs write/close, while workload memory revocation can invoke revoke_memory concurrently. FullSorter access is now locked, but current_writer lifetime/access is not protected.
- Lifecycle: The new _closed flag is guarded by the sorter mutex and prevents late spills after close; no separate lifecycle issue found beyond the unsynchronized current_writer access.
- Config/compatibility/parallel paths: No new configs, storage formats, or FE/BE protocol changes. No additional parallel code path issue found.
- Error handling and observability: Existing Status propagation is preserved; no new observability requirement identified.
- Tests: No new test coverage was added for concurrent spill/write/close. Given the race being fixed, a targeted concurrent test or regression coverage would be valuable.
- Performance: The new mutex serializes spill/write/close and can block revocation behind file I/O, but that appears acceptable for correctness; the blocking issue is the uncovered shared_ptr race.
User focus: no additional user-provided review focus was supplied.
|
|
||
| auto* sort_writer = dynamic_cast<VIcebergSortWriter*>(_writer->current_writer().get()); | ||
| if (!sort_writer || !sort_writer->sorter()) { | ||
| if (!sort_writer) { |
Contributor
There was a problem hiding this comment.
This still reads _writer->current_writer() without synchronization before the new VIcebergSortWriter mutex can help. current_writer() returns a reference to _current_writer, while the async writer thread assigns _current_writer = writer in VIcebergTableWriter::_write_prepared_block() after each write. Workload memory revocation can enter this path concurrently with that assignment, so the shared_ptr object itself is read and written at the same time, which is undefined behavior and can still cause the spill-time crash this PR is trying to fix. Please protect _current_writer with the same synchronization boundary (or return a synchronized copy / atomic shared_ptr) before doing the dynamic_cast.
|
|
||
| #include <cstdint> | ||
| #include <limits> | ||
| #include <mutex> |
Contributor
There was a problem hiding this comment.
Removing <limits> here drops the only explicit include that made this component's std::numeric_limits uses visible. viceberg_sort_writer.cpp still calls std::numeric_limits in _get_spill_batch_size() and _calc_max_merge_streams(), but it does not include <limits> itself, so this now relies on unrelated transitive includes and can fail with include-order changes. Please add #include <limits> to the .cpp or keep it in this header.
Contributor
Author
|
/review |
Contributor
|
OpenCode automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes based on the already-open inline thread that still applies to the latest diff. I did not add a duplicate inline comment for it.
Existing unresolved issue:
be/src/exec/sink/writer/iceberg/viceberg_sort_writer.hremoved the only explicit<limits>include whileviceberg_sort_writer.cppstill usesstd::numeric_limits. This remains a compile-hygiene risk and should be fixed by including<limits>in the.cppor keeping it in the header.
Existing issue that appears addressed by the latest commit:
- The prior unsynchronized
_current_writeraccess is now changed toatomic_shared_ptrwithcurrent_writer()returning a by-value snapshot, and the revoke path keeps that snapshot alive while operating on the sort writer.
Critical checkpoint conclusions:
- Goal/test: The goal is to avoid races between Iceberg sort writer writes and spill revocation. The mutex plus atomic current-writer snapshot mostly addresses the intended race. No new test or manual verification evidence was added in the PR.
- Scope: The latest PR diff is focused on the Iceberg spill writer race.
- Concurrency:
FullSorteraccesses are serialized by_sorter_mutex, and_current_writeris now loaded/stored atomically. Holding the mutex across spill/write/close is conservative but avoids concurrent sorter mutation. - Lifecycle:
_closedis protected by the same mutex and makes late revoke spills no-op after close. No new static initialization or circular ownership hazard found. - Configuration/compatibility: No config, storage format, or FE/BE protocol compatibility changes found.
- Parallel paths: The affected spill memory paths now use
VIcebergSortWriteraccessors instead of raw sorter access. - Conditional checks: New null/type checks are straightforward.
- Test coverage: No test was added for the concurrency fix; residual risk remains because this is a race-prone path.
- Observability: No new observability needed beyond existing status/log paths for this narrow fix.
- Transactions/persistence/data writes: The change affects write/spill serialization but does not alter transaction metadata or persistence. The unresolved include issue should be fixed before merge.
- Performance: The coarse mutex can block revocation memory queries behind write/spill I/O, but I did not find a correctness blocker from that pattern in this PR.
User focus: No additional user-provided review focus was supplied.
Contributor
Author
|
run buildall |
zhangstar333
force-pushed
the
iceberg_spill_error
branch
from
5a8f3bf to
2dfd2dd
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31560 ms |
Contributor
TPC-DS: Total hot run time: 173094 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes because the existing inline include-hygiene issue is still unresolved. I did not add duplicate inline comments.
Existing unresolved issue:
be/src/exec/sink/writer/iceberg/viceberg_sort_writer.hremoved the only explicit<limits>include, whilebe/src/exec/sink/writer/iceberg/viceberg_sort_writer.cppstill usesstd::numeric_limitsin_get_spill_batch_size()and_calc_max_merge_streams(). Please resolve the already-open inline thread by adding#include <limits>to the.cppor keeping it in the header.
Previously raised concurrency issue status:
- The earlier
_current_writerrace appears addressed in the latest commit:_current_writeris now anatomic_shared_ptr,current_writer()returns a by-value snapshot, andrevoke_memory()keeps that snapshot alive while callingtrigger_spill(). I found no distinct remaining instance of that same race.
Critical checkpoint conclusions:
- Goal/test: The goal is to fix Iceberg sort-writer races between write and spill revocation. The latest code largely accomplishes the synchronization goal, but the PR still has no new automated or manual test evidence for this race-prone path.
- Scope: The change is small and focused on Iceberg spill writer synchronization.
- Concurrency: This PR does involve concurrency between the async writer path and memory revocation.
FullSorteraccess is serialized with_sorter_mutex;_closedis protected by the same mutex;_current_writeris now loaded/stored atomically. Holding the mutex across write/spill/close is conservative but acceptable for correctness in this narrow fix. - Lifecycle/static initialization: No new static initialization hazard found. The by-value current-writer snapshot preserves object lifetime during revoke, and
_closedprevents late spills after close. - Config/compatibility: No new config, storage format, or FE/BE protocol compatibility change.
- Parallel paths: The affected Iceberg spill memory paths now use
VIcebergSortWriteraccessors rather than rawsorter()access. I found no other modified parallel path requiring the same change. - Conditional checks: The new null/type checks are straightforward; no non-obvious condition requiring more explanation was found.
- Tests/results: No regression/unit test or result file was added. Residual risk remains because this is a concurrency bug fix.
- Observability: Existing status/logging paths appear sufficient for this narrow change; no new metric/log requirement found.
- Transactions/persistence/data writes: The change serializes write/spill operations but does not change transaction metadata or persistence.
- Performance: The coarse mutex may block memory queries/spill behind write or close work, but I did not find a correctness blocker from that tradeoff.
- Other issues: The unresolved
<limits>include issue remains and should be fixed before merge.
User focus: No additional user-provided review focus was supplied.
Gabriel39
approved these changes
May 25, 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:
spill thread with write thread is different, so add mutex in the write function.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)