github-actions[bot] commented on code in PR #68125:
URL: https://github.com/apache/doris/pull/68125#discussion_r4044578592
##########
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:
This declares VERSION to be a logical read-time constant, but logical direct
row-ID reads do not materialize it. For a partial row-store MOW table whose
`row_columns_uids` omit VERSION, a short-circuit point query selecting
`__DORIS_VERSION_COL__` leaves that UID in `missing_cids`; `PointQueryExecutor`
passes bare options to `seek_and_read_by_rowid()`, which calls
`new_column_iterator()` and returns the stored `0` without
`_replace_version_col_if_needed()`. Please propagate the rowset read context or
synthesize VERSION for logical direct reads while keeping intentionally
physical internal callers explicit, and add a partial-row-store point-query
test.
##########
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:
The new helper defines VERSION as a logical singleton-rowset constant, but
this statistics gate still treats its physical `[0,0]` ZoneMap as usable. With
`show_hidden_columns=true` and `enable_pushdown_minmax_on_unique=true`,
`MIN/MAX(__DORIS_VERSION_COL__)` selects `VStatisticsIterator`; that iterator
reads the physical ZoneMap because `new_column_iterator()` only synthesizes
COMMIT_TSO, so a rowset at version 7 reports `0/0` instead of `7/7`. Please
either materialize every read-time constant in the statistics path or disable
this fast path for those aggregate columns, and add a focused MIN/MAX 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]
