github-actions[bot] commented on code in PR #62899:
URL: https://github.com/apache/doris/pull/62899#discussion_r3193437691
##########
be/src/exec/operator/spill_iceberg_table_sink_operator.cpp:
##########
@@ -58,11 +58,11 @@ size_t
SpillIcebergTableSinkLocalState::get_reserve_mem_size(RuntimeState* state
}
auto* sort_writer =
dynamic_cast<VIcebergSortWriter*>(_writer->current_writer().get());
- if (!sort_writer || !sort_writer->sorter()) {
+ if (!sort_writer) {
Review 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`.
##########
be/src/exec/sink/writer/iceberg/viceberg_sort_writer.h:
##########
@@ -18,7 +18,7 @@
#pragma once
#include <cstdint>
-#include <limits>
+#include <mutex>
Review 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.
--
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]
