github-actions[bot] commented on code in PR #67019:
URL: https://github.com/apache/doris/pull/67019#discussion_r3851312559
##########
be/src/storage/segment/segment.cpp:
##########
@@ -970,18 +975,19 @@ Status Segment::new_column_iterator(const TabletColumn&
tablet_column,
}
Status Segment::get_column_reader(int32_t col_uid,
std::shared_ptr<ColumnReader>* column_reader,
- OlapReaderStatistics* stats, const
io::IOContext* source_io_ctx,
- std::optional<Field> const_value) {
- RETURN_IF_ERROR(_create_column_meta_once(stats, source_io_ctx));
+ const StorageReadOptions& read_options) {
+ auto* stats = read_options.stats;
+ RETURN_IF_ERROR(_create_column_meta_once(stats, &read_options.io_ctx));
SCOPED_RAW_TIMER(&stats->segment_create_column_readers_timer_ns);
// The column is not in this segment, return nullptr
if (!_tablet_schema->has_column_unique_id(col_uid)) {
*column_reader = nullptr;
return Status::Error<ErrorCode::NOT_FOUND, false>("column not found in
segment, col_uid={}",
col_uid);
}
- return _column_reader_cache->get_column_reader(col_uid, column_reader,
stats, source_io_ctx,
- std::move(const_value));
+ return _column_reader_cache->get_column_reader(col_uid, column_reader,
stats,
+ &read_options.io_ctx,
+
_read_time_const_value(col_uid, read_options));
}
Review Comment:
[P1] Keep bare reader lookups from poisoning the new read-time constants.
ColumnReaderCache is keyed only by (uid,path) and returns a cached entry before
considering the const value passed here. Partial-update fetches load the same
cached Segment with bare StorageReadOptions and can cache the physical
VERSION/COMMIT_TSO placeholder first; a later query supplies the real value but
receives that physical reader, so segment pruning/common-expression proofs and
COMMIT_TSO projection can still use 0. Concurrent misses also construct outside
the cache lock and race both representations into this key. Please cache the
physical reader and wrap constants per read, or include the
representation/value in the key, with sequential and concurrent cache-order
regressions on one Segment.
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -459,17 +459,30 @@ Status ColumnReader::read_page(const
ColumnIteratorOptions& iter_opts, const Pag
return PageIO::read_and_decompress_page(opts, handle, page_body, footer);
}
-Status ColumnReader::get_row_ranges_by_zone_map(
- const AndBlockColumnPredicate* col_predicates,
- const std::vector<std::shared_ptr<const ColumnPredicate>>*
delete_predicates,
- RowRanges* row_ranges, const ColumnIteratorOptions& iter_opts) {
- std::vector<uint32_t> page_indexes;
- RETURN_IF_ERROR(
- _get_filtered_pages(col_predicates, delete_predicates,
&page_indexes, iter_opts));
- RETURN_IF_ERROR(_calculate_row_ranges(page_indexes, row_ranges,
iter_opts));
+Status ColumnReader::get_page_zone_map_count(const ColumnIteratorOptions&
iter_opts,
+ size_t* count) {
+ DORIS_CHECK(count != nullptr);
+ if (_zone_map_index == nullptr) {
+ *count = 0;
+ return Status::OK();
+ }
+ RETURN_IF_ERROR(_load_zone_map_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
+ RETURN_IF_ERROR(_load_ordinal_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
+ *count = _zone_map_index->num_pages();
return Status::OK();
}
+Status ColumnReader::get_page_zone_map(size_t page_index, RowRange* rows,
+ segment_v2::ZoneMap* zone_map) {
+ DORIS_CHECK(rows != nullptr && zone_map != nullptr);
+ DORIS_CHECK(_zone_map_index != nullptr && page_index <
_zone_map_index->num_pages());
+ const auto index = cast_set<uint32_t>(page_index);
+ *rows = RowRange(_ordinal_index->get_first_ordinal(index),
+ _ordinal_index->get_last_ordinal(index) + 1);
Review Comment:
[P2] Validate page-zone-map and ordinal-index parity before walking pages.
get_page_zone_map_count() loads both indexes but returns only the zone-map
count, while this accessor checks only that count before
get_first_ordinal()/get_last_ordinal() index the independent ordinal vector
with unchecked operator[]. The removed expression path explicitly bounded
page_index by num_data_pages(); a too-large zone-map index can now read out of
bounds instead of failing that explicit ordinal bound. Please compare the page
counts after load and return corruption before publishing ranges, with a
mismatched-index regression.
--
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]
