github-actions[bot] commented on code in PR #68521:
URL: https://github.com/apache/doris/pull/68521#discussion_r4111551276
##########
be/src/storage/segment/segment.cpp:
##########
@@ -149,11 +162,14 @@ 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.
- if (static_cast<int32_t>(ordinal) == schema.commit_tso_ordinal()) {
- continue;
+ // A hidden placeholder column (version / commit-tso / binlog-tso)
carries an on-disk
+ // zonemap that describes the placeholder, not the value rows come
back with, so a pushed
+ // min/max/count aggregate must not answer from it. Bail the whole
pushed aggregate. These
+ // columns are only in the read schema when explicitly referenced, so
plain aggregates that
+ // do not touch them are unaffected.
+ if (segment->placeholder_effective_value(static_cast<int>(ordinal),
schema, read_options)) {
Review Comment:
[P2] Keep placeholder columns out of forced MIN/MAX statistics reads
On the current target branch, `force_pushdown_zonemap_minmax` makes the
caller skip `segment_zone_maps_can_answer_agg` entirely. After the clean merge,
a UNIQUE-table query with both MIN/MAX pushdown settings enabled can therefore
run `MIN/MAX(__DORIS_VERSION_COL__)` through `VStatisticsIterator`; that
iterator reads the physical `[0,0]` zone map and never reaches
`_replace_version_col_if_needed`, so it returns 0 instead of the effective
rowset version. Please make placeholder-backed columns veto the statistics
iterator even in forced mode, and cover that exact setting/hidden aggregate in
the regression.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1153,10 +1153,11 @@ Status
SegmentIterator::_get_row_ranges_from_conditions(RowRanges* condition_row
_opts.target_cast_type_for_variants, _opts)) {
continue;
}
- if (_segment->is_tso_placeholder_col(cid, *_schema, _opts)) {
- // skip untrustworthy tso placeholder zonemap
- // if possible already be pruned as a whole before,
- // so just skip
+ if (_segment->placeholder_effective_value(cid, *_schema,
_opts).has_value()) {
+ // A hidden placeholder column (version / commit-tso /
binlog-tso) holds one
Review Comment:
[P2] Skip physical indexes for substituted placeholder columns
This check is reached only after `_apply_inverted_index`, and bloom filters
likewise run before this zone-map loop. Those indexes contain the stored
placeholder, not the value produced at read time. For example, Doris accepts an
inverted index on the hidden BIGINT version column; a rowset written under that
schema indexes 0, so `WHERE __DORIS_VERSION_COL__ = <that rowset's positive
version>` passes the synthesized segment check but is reduced to an empty
bitmap before `_replace_version_col_if_needed` runs. Please bypass physical
pre-read indexes for columns with `placeholder_effective_value` (or consume
their predicates against the synthesized singleton first), and add an indexed
hidden-column 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]
