HappenLee commented on code in PR #68097:
URL: https://github.com/apache/doris/pull/68097#discussion_r4036884374
##########
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:
Confirmed this is a real correctness issue, but it predates this PR and will
be handled in a separate follow-up PR.
I checked the pre-PR base `846d9b2ebfc5`: [ordered
compaction](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/compaction/compaction.cpp#L461)
already links the singleton files without materializing per-row TSOs and
produces a spanning-version rowset. The [old data-reader
path](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L946)
already substitutes a logical constant only for a single-version rowset with
assigned TSO. Consequently, a cold read of the linked multi-version output can
expose the physical zero before this PR as well. The compaction implementation
is unchanged by this PR.
This is tracked separately for a producer-side fix (exclude such
placeholder-bearing inputs from ordered link compaction, or materialize their
per-row TSOs), with the requested distinct-publish-TSO coverage. It remains an
outstanding bug; I am not claiming it is fixed by the reader changes here.
##########
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:
Confirmed both metadata-loss paths are pre-existing bugs and will be
addressed in separate follow-up work, rather than in this reader-entrypoint PR.
At the pre-PR base `846d9b2ebfc5`,
[IndexBuilder](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/task/index_builder.cpp#L348)
already constructs fresh metadata without copying the source commit TSO.
[Snapshot rowset-ID
conversion](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/snapshot/snapshot_manager.cpp#L343)
likewise creates the writer context and calls `add_rowset` without preserving
it; [the existing add_rowset
implementation](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/rowset/beta_rowset_writer.cpp#L845)
links files without copying TSO. All three implementations are unchanged by
this PR.
The [pre-PR
reader](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L946)
also requires an assigned TSO to substitute the singleton's logical value.
Thus losing it to `[-1,-1]` already makes a cold read fall back to the on-disk
placeholder, independently of this change.
These paths are tracked separately for preserving source metadata and adding
index add/drop plus rowset-ID conversion coverage. They are not being marked as
fixed here.
##########
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:
Fixed in [6363e04f6bd](https://github.com/apache/doris/commit/6363e04f6bd),
included in current head `b21ac4087032`.
`Segment::new_iterator` now retains the TSO reader obtained during
first-phase pruning and explicitly reuses it for predicate simplification,
independently of physical-cache warmup. TSO is skipped in the physical-reader
cache traversal. `prune_predicates_by_zone_map` now uses virtual
`has_zone_map()` / `get_segment_zone_map()` and the existing `is_always_true()`
logic, so the logical constant's ZoneMap participates without putting a
scan-specific reader in the shared cache.
[Tests](https://github.com/apache/doris/blob/b21ac408703240c9237f5b3ba6a7c7c586de863e/be/test/storage/segment/segment_iterator_expr_zonemap_test.cpp#L471)
cover cold/warm physical caches, TSO projected/not projected, removal from
both predicate representations, and the no-need-read flag. They also verify
that other expression dependencies still require TSO and that
multi-version/unknown-TSO reads retain the physical behavior. These cases
passed in the final 85-test ASAN run.
##########
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:
Fixed in
[b21ac408703](https://github.com/apache/doris/commit/b21ac408703240c9237f5b3ba6a7c7c586de863e)
by removing the independent point-query TSO fetch and reusing the existing
missing-column path.
Point queries now always treat TSO as unavailable from JSONB. Explicit
row-store projections prevent both decoding and default-filling its
placeholder; an empty projection skips JSONB entirely. TSO and other missing
columns share the normal segment lookup and `seek_and_read_by_rowid`, with the
pinned source rowset, query `IOContext` and `_remote_scan_cache_write_limiter`.
All point-query reads now write to `_profile_metrics.read_stats`, the same
object consumed by `print_profile`, so the former `_read_stats`/profile split
is also removed.
The tests cover TSO-only/mixed projections, full/partial/no row store,
ordinary missing-column restrictions with column-store access disabled, and
delete-sign filtering. Physical multi-version TSO tests verify zero and
exhausted cache-write budgets, an unlimited control, correct TSO values, no
cache admission when limited, and query byte/timer accounting. They use the
real `CachedRemoteFileReader` with a local file standing in for object storage;
this is not a live-cloud integration test.
The final ASAN run passed 85 tests in 8 suites. SQL regression coverage was
extended but not run locally.
--
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]
