Copilot commented on code in PR #65907:
URL: https://github.com/apache/doris/pull/65907#discussion_r3629467506
##########
be/src/storage/rowid_conversion.h:
##########
@@ -40,6 +41,15 @@ class RowIdConversion {
// resize segment rowid map to its rows num
Status init_segment_map(const RowsetId& src_rowset_id, const
std::vector<uint32_t>& num_rows) {
for (size_t i = 0; i < num_rows.size(); i++) {
+ auto src_segment = std::pair<RowsetId, uint32_t> {src_rowset_id,
cast_set<uint32_t>(i)};
+ auto iter = _segment_to_id_map.find(src_segment);
+ // Each segment-group reader initializes all source segments, so
reuse existing maps.
+ if (iter != _segment_to_id_map.end()) {
+ DORIS_CHECK_LT(iter->second, _segments_rowid_map.size());
+ DORIS_CHECK_EQ(_segments_rowid_map[iter->second].size(),
num_rows[i]);
+ continue;
+ }
Review Comment:
`init_segment_map()` returns `Status`, but it uses `DORIS_CHECK*` for
recoverable/state-validation conditions (existing mapping bounds/size match,
duplicate insert). If these checks trip in production they can terminate the
process / throw rather than returning an error. Prefer converting these to
`return Status::InternalError(...)` / `Status::InvalidArgument(...)` (or
similar) so compaction/read can fail gracefully without crashing the BE.
##########
be/src/storage/rowset/vertical_beta_rowset_writer.cpp:
##########
@@ -140,8 +140,11 @@ Status
VerticalBetaRowsetWriter<T>::_flush_columns(segment_v2::SegmentWriter* se
key_bounds.set_min_key(min_key.to_string());
key_bounds.set_max_key(max_key.to_string());
this->_segments_encoded_key_bounds.emplace_back(std::move(key_bounds));
- this->_segment_num_rows.resize(_cur_writer_idx + 1);
- this->_segment_num_rows[_cur_writer_idx] =
_segment_writers[_cur_writer_idx]->row_count();
+ const auto segment_id = segment_writer->get_segment_id();
+ if (segment_id >= this->_segment_num_rows.size()) {
+ this->_segment_num_rows.resize(segment_id + 1);
+ }
+ this->_segment_num_rows[segment_id] = segment_writer->row_count();
Review Comment:
`_segment_num_rows` is now indexed by `segment_id`, but
`_segments_encoded_key_bounds` is still appended sequentially via
`emplace_back`. If `segment_writer->get_segment_id()` can differ from the
append order (the fact this change was needed suggests it can), key-bounds and
per-segment row counts can become misaligned by segment index, corrupting
rowset meta invariants. Consider storing key bounds by `segment_id` as well
(resize + assign at index), matching the `_segment_num_rows` approach.
##########
be/src/storage/rowid_conversion.h:
##########
@@ -59,9 +69,10 @@ class RowIdConversion {
->consumption()));
}
- uint32_t id = static_cast<uint32_t>(_segments_rowid_map.size());
- _segment_to_id_map.emplace(std::pair<RowsetId, uint32_t>
{src_rowset_id, i}, id);
- _id_to_segment_map.emplace_back(src_rowset_id, i);
+ uint32_t id = cast_set<uint32_t>(_segments_rowid_map.size());
+ auto insert_result = _segment_to_id_map.emplace(src_segment, id);
+ DORIS_CHECK(insert_result.second);
+ _id_to_segment_map.push_back(src_segment);
Review Comment:
`init_segment_map()` returns `Status`, but it uses `DORIS_CHECK*` for
recoverable/state-validation conditions (existing mapping bounds/size match,
duplicate insert). If these checks trip in production they can terminate the
process / throw rather than returning an error. Prefer converting these to
`return Status::InternalError(...)` / `Status::InvalidArgument(...)` (or
similar) so compaction/read can fail gracefully without crashing the BE.
--
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]
