github-actions[bot] commented on code in PR #68125:
URL: https://github.com/apache/doris/pull/68125#discussion_r4034334657
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1127,6 +1127,9 @@ Status
SegmentIterator::_get_row_ranges_from_conditions(RowRanges* condition_row
RowRanges bf_row_ranges = RowRanges::create_single(num_rows());
for (auto& cid : cids) {
DCHECK(_opts.col_id_to_predicates.count(cid) > 0);
+ if (_segment->get_read_time_constant_value(cid, *_schema,
_opts).has_value()) {
+ continue;
Review Comment:
The new guards cover Bloom filters and page ZoneMaps, but the same predicate
can still be consumed by `_apply_inverted_index_on_column_predicate()` using
physical postings. Hidden BIGINT columns are indexable, and the online builder
scans them with default `version=(0,0)`, so an index on `__DORIS_VERSION_COL__`
records 0. On a singleton rowset whose logical version is 7, `WHERE
__DORIS_VERSION_COL__ = 7` now passes segment pruning but the inverted index
empties `_row_bitmap` before `_replace_version_col_if_needed()` can run. Please
exclude every column for which `get_read_time_constant_value()` is set from
inverted-index creation/application too, and add a physical-0/logical-7 test.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -423,18 +452,22 @@ Status Segment::new_iterator(ReadSchemaSPtr schema, const
StorageReadOptions& re
// col_id_to_predicates is keyed by read-schema ordinal.
int32_t column_id = entry.first;
const TabletColumn& col = *schema->column(column_id);
- std::shared_ptr<ColumnReader> reader;
- // __DORIS_COMMIT_TSO_COL__ on a single-version segment stores a 0
placeholder on disk
- // (replaced with the rowset's real commit_tso at read time). Its
on-disk zonemap [0,0]
- // must not drive segment-level pruning, so build a
ConstantColumnReader carrying the real
- // commit_tso to prune against the real value instead.
- std::optional<Field> const_value;
- if (read_options.version.first == read_options.version.second &&
- column_id == schema->commit_tso_ordinal() &&
read_options.commit_tso.end_tso() != -1) {
- const_value =
Field::create_field<TYPE_BIGINT>(read_options.commit_tso.end_tso());
+ if (auto value = get_read_time_constant_value(column_id, *schema,
read_options);
+ value.has_value()) {
Review Comment:
This branch no longer installs the COMMIT_TSO constant reader while pruning.
Because the cached Segment's reader cache is UID-only, a concurrent
fixed-column partial-update/row-ID read can use bare options to cache the
physical placeholder after this check but before `Segment::new_iterator()`
reaches its final internal `iter->init()`; that init requests commit 100 but
receives the cached physical reader, so a predicate kept here using 100 is
evaluated row-by-row as 0. The old code eagerly requested the constant reader
at this point, so this empty-cache ordering is introduced here. Please avoid
caching option-dependent constant and physical readers under the same key (or
synthesize COMMIT_TSO in the iterator) and add an interleaving/cache-order test.
--
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]
