mrhhsg commented on code in PR #68125:
URL: https://github.com/apache/doris/pull/68125#discussion_r4226953037
##########
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:
Addressed in 443f87d76f4 and kept at the current head.
`SegmentIterator::_init_index_iterators` leaves `_index_iterators[cid]` empty
for every column whose `Segment::get_read_time_constant_value()` is set, so
neither `_apply_inverted_index_on_column_predicate()` nor the expression-index
path can consume the physical postings of a hidden column on a singleton
rowset; the predicate is evaluated row by row against the synthesized value.
`SegmentIteratorExprZonemapTest.VersionPredicateSkipsPhysicalInvertedIndex`
writes a segment whose VERSION column and inverted index hold the physical 0,
reads it as version 7 with `__DORIS_VERSION_COL__ = 7` through
`Segment::new_iterator`, and asserts every row comes back as 7 with
`rows_inverted_index_filtered == 0`. Multi-version rowsets have no read-time
constant and keep using their index, which
`test_build_index_read_time_hidden_column` covers end to end.
##########
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:
Addressed in fcd3c13f8d4 / 360150637fe and kept at the current head.
`ColumnReaderCache::get_column_reader` neither looks up nor inserts the
UID-keyed cache when a `const_value` is passed, so a constant COMMIT_TSO reader
can never be served from, or leak into, the physical reader cache whichever
read path runs first; the option-dependent reader is built per request.
`Segment::new_iterator` prunes COMMIT_TSO predicates straight from
`read_options.commit_tso` without touching the cache.
Covered by `CommitTsoReaderIgnoresCachedPhysicalReader` (physical reader
cached first, the logical read still returns the TSO),
`CommitTsoReaderDoesNotPollutePhysicalReaderCache` (logical reader first, the
physical read still returns 0) and
`NewIteratorPrunesCommitTsoByReadOptionValue`.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -149,9 +159,8 @@ Status segment_zone_maps_can_answer_agg(Segment* segment,
const ReadSchema& sche
const StorageReadOptions&
read_options, bool* usable) {
*usable = true;
for (size_t ordinal = 0; ordinal < schema.num_block_columns(); ++ordinal) {
- // The commit-tso column is only served correctly once its reader is
created with the
- // rowset's commit_tso as a const value. Creating it here without one
would cache a reader
- // that hands every later read the on-disk placeholder instead.
+ // The statistics iterator reads segment metadata without
StorageReadOptions and therefore
+ // cannot materialize the rowset's commit_tso in place of the on-disk
placeholder.
Review Comment:
Addressed in 360150637fe and kept at the current head:
`segment_zone_maps_can_answer_agg` sends VERSION / BINLOG_TSO (which only
`SegmentIterator` materializes) to the row iterator, including under forced
pushdown, and keeps the statistics path for an assigned singleton COMMIT_TSO
through its constant reader. Covered by
`VersionMinMaxFallsBackFromStatisticsIterator`,
`BinlogTsoMinMaxFallsBackFromStatisticsIterator`,
`CommitTsoMinMaxUsesStatisticsIterator` and the MIN/MAX pushdown cases of
`test_point_query_read_time_hidden_columns`.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -410,6 +419,25 @@ bool Segment::is_tso_placeholder_col(int cid, const
ReadSchema& schema,
return cid == schema.tso_ordinal();
}
+std::optional<Field> Segment::get_read_time_constant_value(
+ int cid, const ReadSchema& schema, const StorageReadOptions&
read_options) const {
+ if (read_options.version.first != read_options.version.second) {
+ return std::nullopt;
+ }
+ if (cid == schema.version_ordinal()) {
+ return Field::create_field<TYPE_BIGINT>(read_options.version.second);
Review Comment:
Addressed in b8c448f781c / ac4694b3fe8 / c2ba3bafaf9: short-circuit point
queries and row-ID reads keep the owning rowset and resolve VERSION /
COMMIT_TSO from it (singleton) or from column storage (multi-version) instead
of the physical placeholder, for both the partial-row-store `missing_cids` path
and the full-row-store JSONB path. Covered by `PointQueryHiddenColumnTest.*`
and the partial-row-store point-query cases of
`test_point_query_read_time_hidden_columns`.
--
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]
