github-actions[bot] commented on code in PR #68097:
URL: https://github.com/apache/doris/pull/68097#discussion_r4030171694
##########
be/src/storage/segment/segment.cpp:
##########
@@ -532,9 +514,15 @@ Status Segment::new_iterator(ReadSchemaSPtr schema, const
StorageReadOptions& re
if (ordinal < 0) {
continue;
}
+ // This enumeration contains physical readers only. A cached TSO
placeholder must
+ // not make a logical predicate look unconditionally true.
+ auto reader = it.second;
+ if (ordinal == schema->commit_tso_ordinal()) {
Review Comment:
[P2] Make commit-TSO predicate simplification reachable. A cold logical
`ConstantColumnReader` is never cached, so this loop never sees it. If a
physical reader was warmed, the replacement below still invokes the non-virtual
base `prune_predicates_by_zone_map`, which returns immediately because the
constant reader has no physical `_zone_map_index`. Consequently an always-true
predicate such as `commit_tso > 0` survives and synthesizes/evaluates the same
8-byte value for every row. Please simplify the commit-TSO ordinal explicitly
against its logical `[tso,tso]` ZoneMap (with a test that proves the true
predicate/column is removed from row evaluation).
##########
be/src/storage/segment/segment.cpp:
##########
@@ -1012,9 +983,30 @@ Status Segment::new_column_iterator(const TabletColumn&
tablet_column,
return Status::OK();
}
-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) {
+Status Segment::get_column_reader(const TabletColumn& col,
+ std::shared_ptr<ColumnReader>* column_reader,
+ const StorageReadOptions& read_options) {
+ // The on-disk column is a placeholder until compaction materializes
per-row commit TSOs.
+ // Resolve the logical value before any metadata/cache lookup, including
for older segments
+ // that do not contain this column. Never put this reader in the physical
metadata cache:
+ // unpublished reads and published reads can share the same Segment object.
+ if (col.name() == COMMIT_TSO_COL && read_options.version.first ==
read_options.version.second &&
+ read_options.commit_tso.end_tso() != -1) {
+ DORIS_CHECK_GT(read_options.commit_tso.end_tso(), 0);
+ DORIS_CHECK_EQ(read_options.commit_tso.start_tso(),
read_options.commit_tso.end_tso());
+ *column_reader = std::make_shared<ConstantColumnReader>(
+
Field::create_field<TYPE_BIGINT>(read_options.commit_tso.end_tso()));
+ return Status::OK();
+ }
+ // An unassigned TSO (-1) is expected on unpublished internal reads.
Multi-version rowsets
+ // contain materialized TSOs, which must be read from the physical column.
+ return get_physical_column_reader(col, column_reader, read_options.stats,
&read_options.io_ctx);
Review Comment:
[P1] Keep linked compaction from exposing placeholder commit TSOs.
Ordered-data compaction can reach a TSO-enabled DUP base tablet and
`link_files_to` several single-version rowsets into one spanning-version rowset
without rewriting their segment/row-store data. Those source files still
contain the sink-time `0` placeholder, so this branch now selects that physical
value; direct hidden-column reads return `0`, and a time-travel predicate such
as `commit_tso < target` can admit rows newer than the snapshot. Please exclude
commit-TSO schemas from ordered link compaction (or materialize a per-row TSO
representation there) and cover two distinct published TSOs after ordered
compaction.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -1012,9 +983,30 @@ Status Segment::new_column_iterator(const TabletColumn&
tablet_column,
return Status::OK();
}
-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) {
+Status Segment::get_column_reader(const TabletColumn& col,
+ std::shared_ptr<ColumnReader>* column_reader,
+ const StorageReadOptions& read_options) {
+ // The on-disk column is a placeholder until compaction materializes
per-row commit TSOs.
+ // Resolve the logical value before any metadata/cache lookup, including
for older segments
+ // that do not contain this column. Never put this reader in the physical
metadata cache:
+ // unpublished reads and published reads can share the same Segment object.
+ if (col.name() == COMMIT_TSO_COL && read_options.version.first ==
read_options.version.second &&
Review Comment:
[P1] Preserve commit TSO across linked rowset rebuilds. `IndexBuilder` keeps
the input singleton version and links its files but omits
`input_rowset_meta->commit_tso()` from the fresh metadata. Snapshot rowset-ID
conversion (used by clone, restore, and storage migration) likewise hard-links
through `BaseBetaRowsetWriter::add_rowset`, whose context/add/build path never
copies the source TSO. Both outputs therefore report `[-1,-1]`, this condition
falls through, and the linked segment's sink-time `0` placeholder becomes
visible, changing hidden-column results and historical visibility without
changing data. Please preserve the source TSO in both rebuild paths and cover
index add/drop plus rowset-ID conversion.
##########
be/src/service/point_query_executor.cpp:
##########
@@ -605,11 +615,24 @@ Status PointQueryExecutor::_lookup_row_data() {
StorageReadOptions storage_read_options;
storage_read_options.stats = &_read_stats;
storage_read_options.io_ctx = io_ctx;
+ storage_read_options.tablet_schema =
rowset->tablet_schema();
+ storage_read_options.rowset_id = rowset->rowset_id();
+ storage_read_options.version = rowset->version();
+ storage_read_options.commit_tso = rowset->commit_tso();
RETURN_IF_ERROR(segment->seek_and_read_by_rowid(*_tablet->tablet_schema(), slot,
row_ids,
column,
storage_read_options, iter));
}
}
+ if (tso_idx != -1) {
+ const auto& loc = _row_read_ctxs[i]._row_location.value();
+ auto& column = result_columns[tso_idx];
+ column->resize(old_tso_rows);
+ const auto* slot = _reusable->tuple_desc()->slots()[tso_idx];
+ RETURN_IF_ERROR(BaseTablet::fetch_value_by_rowids(
Review Comment:
[P2] Preserve the point query's I/O context for this TSO fetch. This call
now bypasses the missing-column path that carries `_read_stats` and
`_remote_scan_cache_write_limiter`; `fetch_value_by_rowids` creates local
statistics and calls `_get_segment_column_iterator` without an input
`IOContext`. For a compacted multi-version rowset the TSO is a physical remote
page read, so it can write through the file cache after the query limiter is
exhausted, and its bytes/timers never reach the point-query profile. Please let
the helper accept caller statistics/context and pass the existing point-query
limiter here (ideally reusing/batching the already located segment), with cloud
multi-version coverage.
--
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]
