Repository navigation
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#68018 Problem Summary: A single-version rowset stores a placeholder in `__DORIS_COMMIT_TSO_COL__`; its logical value comes from the rowset's publish metadata. Some read paths request a physical column reader before the data iterator supplies that value. A cross-column condition such as `commit_tso > 0 OR v = 99` can therefore prune a segment using the placeholder ZoneMap. Historical reads and JSONB-based row-id/point reads can also return the placeholder. Resolve the logical reader in `Segment::get_column_reader` using the source rowset's version and commit TSO, before consulting physical metadata or the cache. Single-version published reads receive a constant reader; multi-version and unpublished internal reads retain physical readers. Keep only physical readers in `ColumnReaderCache`, removing the optional constant parameter and cache-upgrade state. Rename deliberate Variant/storage metadata accesses to `get_physical_column_reader`. Route segment/page expression ZoneMaps, ordinary predicates, predicate simplification, aggregate statistics, data iterators and indexes through this entry point. Supply source rowset metadata to historical and row-id reads. For JSONB historical reads, two-phase fetch and short-circuit point queries, preserve ordinary row-store columns and resolve TSO separately. Point queries requesting TSO bypass the JSONB row cache because it does not retain the source rowset location. This is an alternative to apache#68018's cache-upgrade approach. It retains that PR's same-key LRU replacement fix and constant-reader index no-op: concurrent physical cache misses can still insert the same key, and physical indexes are inapplicable to logical constants. No storage format or FE/BE protocol changes. ### Release note Fix incorrect commit TSO values and filtering in expression pushdown, historical reads, two-phase row-id fetch and point queries. ### Check List (For Author) - Test - [ ] Regression test: added `tso_p0/test_commit_tso_reader_paths`, using runtime rowset metadata as the oracle; not executed because this worktree has no independent FE/BE cluster. - [x] Unit Test: 62 tests from 6 suites passed under ASAN_UT using `run-be-ut.sh -j 48` (`SegmentIteratorExprZonemapTest`, `ConstantColumnIteratorTest`, `ColumnReaderCacheTest`, `HistoricalRowRetrieverTest`, `PointQueryCommitTsoTest`, `RowIdStorageReaderTest`). - [x] Manual test: clang-format 16 check, build hygiene and `git diff --check` passed. BE sources compiled and `doris_be` linked; full `build.sh` was blocked when the kuromoji dictionary generator hit an ASAN double-free in OpenMP initialization. clang-tidy was attempted but blocked by the existing unmatched NOLINT marker in `be/src/core/types.h:576`; it is not reported as passed. - Behavior changed: - [x] Yes. Logical TSO values are used consistently before filtering and when reading JSONB-backed rows; only point queries requesting TSO bypass the row cache. - Does this need documentation? - [x] No. Co-authored-by: yangjie01 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review Please focus on correctness of commit TSO resolution across segment/page ZoneMaps, predicate simplification, statistics, MOW historical reads, row-store two-phase fetch, and short-circuit point queries. Check single-version versus multi-version and unpublished rowsets, physical cache isolation, Variant root-reader dispatch, and whether the implementation is minimal and avoids unnecessary I/O or per-row overhead. This PR replaces #68018's constant-reader cache-upgrade approach. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: two commit-TSO correctness gaps and two hot-path/I/O-contract regressions remain at exact head 865b4313bd2d73ab385de55557428df0e0b8f223.
Findings:
- P1: ordered-data link compaction can turn single-version zero placeholders into a multi-version rowset that this patch reads physically, corrupting direct commit-TSO results and historical filtering.
- P1: IndexBuilder and snapshot rowset-ID conversion link the source files but can drop the singleton rowset's commit-TSO metadata, causing the same physical zero to become visible after index rebuild, clone, restore, or storage migration.
- P2: the new logical-reader lookup does not actually make an always-true commit-TSO predicate simplifiable; it remains in row-level materialization and evaluation.
- P2: the new point-query TSO fetch loses the caller's statistics and I/O context, so physical cloud reads bypass the query cache-write limiter and profile accounting.
The ordinary published-singleton path now consistently supplies the logical constant to data reads, segment/expression ZoneMaps, MIN/MAX statistics, missing-column handling, and index fallback. Normally rewritten multi-version rowsets remain physical, unpublished [-1,-1] rowsets retain the prior behavior, Variant root/path access remains deliberately physical, and logical readers stay out of the shared physical cache. The four inline comments cover the remaining exceptions.
Checkpoint conclusions:
- Goal and tests: the goal is met for ordinary singleton and materialized multi-version rowsets, but not for the two linked-file transformations above. Added tests cover logical singleton reads, false segment pruning, historical/two-phase reads, and local point queries; they do not cover the four reported cases.
- Scope and focus: the central logical/physical reader split is focused, but it assumes all linked rowset producers preserve either per-row values or singleton TSO metadata; two producers and ordered compaction violate that assumption.
- Concurrency: the physical-reader cache mutex, same-key replacement, map/list membership, and gauge accounting remain coherent; I found no new lock-order or deadlock risk.
- Lifecycle and static state: Segment and rowset pins cover synchronous reads and no static initialization issue was introduced. The lifecycle defect is metadata loss during index rebuild and clone/restore/migration rowset-ID conversion.
- Configuration: no new configuration is introduced. Existing ordered-data compaction defaults make its linked-file case reachable.
- Compatibility: no protocol or storage-format incompatibility was found. Legacy absent TSO fields correctly default to
[-1,-1], and the unpublished/rolling-upgrade fallback remains conservative. - Parallel paths: ordinary scans, statistics, index fallback, historical/two-phase reads, point queries, Variant readers, single-version, multi-version, and unpublished sources were traced. The four comments identify the remaining divergent paths.
- Conditions and comments: the singleton condition is valid only while transformations retain rowset TSO metadata, and the multi-version fallback is valid only when compaction materializes per-row TSOs. The linked paths falsify those documented assumptions.
- Coverage: the new unit and regression tests exercise the intended baseline, while ordered link compaction, index rebuild/rowset-ID conversion, always-true predicate removal, and cloud multi-version point-query I/O context remain uncovered.
- Test-result changes: changed expected results and signature adaptations are consistent with the intended logical values. This review was static only; per the review contract, I did not build or execute tests.
- Observability: physical TSO I/O introduced by the point-query path is not charged to the point-query profile and does not inherit its cache-write limiter. No other new metric or logging requirement was identified.
- Transactions and persistence: there is no EditLog or transaction-state change. Commit-TSO metadata nevertheless must survive linked rowset rebuilds because read semantics now depend on it.
- Data writes and failures: normal rewriting compaction materializes correct values and existing status propagation remains intact. Linked transformations preserve files but lose the metadata/value representation required by the new reader decision; I found no new resource leak or deadlock.
- FE/BE variables: no FE-to-BE variable or session/config propagation was added.
- Performance: always-true predicates still synthesize and evaluate an 8-byte constant per row, and point queries can perform unaccounted physical reads with default cache policy. Other constant-reader/index paths do not add material I/O.
- Other checks: no independent nullable-column, ownership, memory-safety, or error-propagation defect was found.
Static-only review: builds and tests were prohibited and were not run. The bounded review converged in Round 3; every final reviewer returned NO_NEW_VALUABLE_FINDINGS, and all candidate findings were adjudicated and duplicate-fenced before this submission.
| // 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()) { |
There was a problem hiding this 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).
There was a problem hiding this comment.
Fixed in 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 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.
| // 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 && |
There was a problem hiding this 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.
There was a problem hiding this 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 already constructs fresh metadata without copying the source commit TSO. Snapshot rowset-ID conversion likewise creates the writer context and calls add_rowset without preserving it; the existing add_rowset implementation links files without copying TSO. All three implementations are unchanged by this PR.
The pre-PR reader 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.
| } | ||
| // 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); |
There was a problem hiding this 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.
There was a problem hiding this 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 already links the singleton files without materializing per-row TSOs and produces a spanning-version rowset. The old data-reader path 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.
| 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( |
There was a problem hiding this 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.
There was a problem hiding this comment.
Fixed in b21ac408703 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.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68097 Problem Summary: On a single-version rowset whose logical commit TSO is 42, TSO > 41 survives segment filtering even though every row satisfies it. Always-true predicate elimination only visits cached physical readers, so it misses an uncached logical TSO reader. Its reader helper also accesses physical zone-map metadata directly, bypassing constant readers. Retain the first-stage TSO reader within new_iterator and reuse it during predicate elimination, independently of physical cache state. Obtain the zone map through the existing reader interface and reuse is_always_true. Keep per-segment predicate copies and existing column-read dependency checks, so projected TSO values are still read. Report null constants as nullable zone maps to keep comparisons conservative. Add comments and unit tests for cache state, predicate removal, data output and physical reader fallback. No performance benchmark was run. ### Release note Avoid redundant row-level commit TSO filters when the segment's logical TSO proves them always true. ### Check List (For Author) - Test: Unit Test - ASAN: 43 tests passed across ConstantColumnIteratorTest, SegmentIteratorExprZonemapTest and ColumnReaderCacheTest - clang-format 16, build hygiene and git diff --check passed - clang-tidy attempted: blocked by the pre-existing unmatched NOLINTEND in core/types.h:576; no diagnostics on changed lines - Behavior changed: Yes (remove redundant filters; query results unchanged) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#68097 Problem Summary: A point query requesting commit TSO fetched that column through an independent helper with local statistics and no query I/O context. For a multi-version rowset with a cold physical TSO page, this bypassed the query's file-cache write limiter and omitted detailed read statistics from its profile. Always classify TSO as unavailable from JSONB. Build explicit row-store and missing-column projections, skip empty JSONB projections, and resolve TSO through the existing missing-column loop with the pinned source rowset. Reuse query I/O context and limiter, and use the same statistics object as print_profile for all point-query reads. Preserve delete-sign reads and restrictions on ordinary missing columns when column-store access is disabled. Remove the independent TSO fetch, placeholder rollback and redundant projection-set copies. ### Release note Preserve point-query file-cache write limits and detailed I/O statistics when reading commit TSO, including physical TSO pages in multi-version rowsets. ### Check List (For Author) - Test: Unit Test: 85 tests in 8 suites passed under ASAN_UT via run-be-ut.sh -j 96. Covers row-store projections, ordinary column restrictions, delete-sign reads, physical multi-version TSO with zero/exhausted cache-write budgets, an unlimited control, query statistics, and the existing logical TSO/cache/historical paths. The real remote-cache reader uses a local file as the object-storage stand-in. Regression test: extended tso_p0/test_commit_tso_reader_paths; not run locally. Manual test: clang-format 16, build hygiene and git diff --check passed. clang-tidy attempted; existing core/types.h:576 unmatched NOLINTEND blocks a clean result. New diagnostics were corrected; the check is not reported as passed. - Behavior changed: Yes. TSO uses the normal missing-column I/O context and profile statistics; JSONB never supplies its value. - Does this need documentation: No.
|
/review Please re-review the complete PR at head This update addresses the previous review's predicate-simplification and point-query I/O findings:
Please focus on row-store projection/default-fill semantics, TSO-only and mixed output slots, the disabled column-store setting, multi-version physical TSO reads, limiter/statistics propagation, rowset lifetime, and whether any unnecessary I/O remains. Also review the rest of the PR for regressions. Validation: 85 ASAN unit tests passed in 8 suites. The physical TSO tests use the real CachedRemoteFileReader with a local object-storage stand-in and cover zero/exhausted cache-write budgets, an unlimited control, and query I/O statistics. Formatting and build hygiene passed. SQL regression coverage was extended but not run; no live cloud integration test was run. clang-tidy remains blocked by the pre-existing unmatched NOLINTEND in core/types.h; newly introduced diagnostics were corrected and rechecked. The two linked-rowset producer issues from the previous review were confirmed to predate this PR and are tracked separately; their implementations are unchanged in this update. |
There was a problem hiding this comment.
Static exact-head review of b21ac408703240c9237f5b3ba6a7c7c586de863e: requesting changes for three P2 issues—one avoidable TSO-only row-store read, one flaky distributed-snapshot test oracle, and one formatter failure.
Critical checkpoints:
- Goal: The logical commit-TSO reader design now handles singleton published rowsets, while multi-version/unpublished rowsets retain physical reads. Point-query projection/default-fill, disabled column-store, mixed slot order, delete sign, pinned-rowset lifetime, limiter/statistics, zonemap, index, aggregate, and Variant paths are otherwise coherent.
- Focusedness: Production changes are reasonably focused around reader dispatch and affected consumers; the cache race fix is locally contained. The stray test reformat is both unnecessary and currently breaks the formatter gate.
- Concurrency:
ColumnReaderCachemap/LRU replacement remains under its shard lock, does not change entry count on same-key races, and retains shared ownership for racing readers; no new lock-order or deadlock issue was found. - Lifecycle: Point lookup retains the exact source rowset through value fetch, including compaction replacement, and segment/iterator/cache ownership has no new cycle or premature release.
- Configuration: No configuration item was added or changed.
- Compatibility: No wire protocol or storage-format change was introduced. The logical/physical reader split is internal, and old placeholder segments continue to route by rowset version/commit-TSO metadata.
- Parallel paths: Regular scans, point queries, two-phase row-ID fetch, historical/BaseTablet reads, statistics iterators, predicates, indexes, and Variant physical readers were traced. The remaining parallel-path problem is the TSO-only TopN lazy fetch called out inline.
- Special conditions: Published singleton rowsets require an assigned positive singleton TSO and use the logical constant; multi-version and unpublished reads use the physical column. Cold/warm cache and always-true/common-expression pruning were rechecked with no further finding.
- Test coverage: The new unit coverage is broad, but it misses the TSO-only two-phase I/O shape, and the SQL oracle does not wait for distributed rowset visibility.
- Test results: Per the PR notes, 85 ASAN unit tests passed across 8 suites; SQL regression and live cloud integration were not run, and clang-tidy remains blocked by a pre-existing issue. I did not run builds/tests under this review-only task. On this exact head, the changed-files clang-format 16 dry run and the live Clang Formatter job fail at the inline test line.
- Observability: Point-query statistics, file-cache statistics, cache-write limiter, and row-ID miss policy propagate on the changed reads; existing counters/logs are sufficient for the reviewed paths.
- Persistence: No EditLog, persisted metadata schema, or failover behavior is changed.
- Data writes: This is a read-path change; rowset publish/compaction producers are unchanged.
- FE/BE variables: No new cross-process variable was added. Existing lazy-materialization row-store flags and rowset read options were traced end to end.
- Performance: TSO-only TopN lazy materialization still reads and decodes a full JSONB row that it immediately discards; see the inline finding. Other point-query TSO-only paths correctly skip JSONB.
- Other: Existing linked-rowset producer findings and the earlier point-query I/O thread were treated as duplicate fences and were not reposted.
Focus response: row-store projection/default fill, TSO-only and mixed slots, disabled column store, multi-version physical reads, limiter/statistics propagation, and rowset lifetime all passed the static re-review apart from the explicit TSO-only two-phase redundant I/O. Review status is complete and static-only for this exact head.
|
|
||
| // if row_store_read_struct not empty, means the line we should read from row_store | ||
| if (!row_store_read_struct.default_values.empty()) { | ||
| const bool use_row_store = !row_store_read_struct.default_values.empty(); |
There was a problem hiding this comment.
[P2] Skip row-store I/O for a TSO-only lazy fetch
For a full-row-store query such as SELECT __DORIS_COMMIT_TSO_COL__ FROM t ORDER BY v LIMIT 10, TopN lazy materialization can send fetch_row_store=true with commit TSO as the only lazy slot. This branch then reads and decodes the JSONB row, but the loop below immediately truncates that placeholder before resolving TSO through seek_and_read_by_rowid, so all of that row-store data-page I/O and decoding is discarded. Please enable this path only when at least one non-TSO lazy slot can consume the JSONB row, and cover the TSO-only two-phase shape.
| ASSERT_NE(entry, nullptr); | ||
| EXPECT_EQ(entry.use_count(), 3); // Cache + local reference | ||
| } // Local reference released | ||
| } // Local reference released |
There was a problem hiding this comment.
[P2] Restore the clang-format output
This changed line is the sole changed-file clang-format 16 violation, and the live Clang Formatter job is failing on this head. Please keep the formatter-produced spacing so the mandatory style gate can pass.
| } // Local reference released | |
| } // Local reference released |
|
/review Please re-review the complete PR at its current head, taking the existing review discussions and replies into account. The regression-test metadata-oracle synchronization issue has been explicitly deferred to a separate follow-up; its thread was resolved as deferred, not as fixed. Please review the BE commit-TSO reader changes and report any remaining or new issues. |
There was a problem hiding this comment.
Static re-review result at b21ac408703240c9237f5b3ba6a7c7c586de863e: NO_NEW_VALUABLE_FINDINGS.
I reviewed all 22 changed paths and the relevant reader, predicate/index, statistics, point-query, row-id, historical-read, cache, and test call chains. One complete normal plus risk-focused round converged. No materially distinct inline issue survived the existing-thread duplicate fence, so I am not adding duplicate inline comments. This comment does not clear the existing requested changes.
Existing discussion state:
- The current head contains the logical ZoneMap/predicate-simplification fix and the point-query I/O-context/statistics/limiter fix; I rechecked their downstream paths and found no distinct recurrence.
- The TSO-only lazy row-id path still performs avoidable JSONB row-store I/O; that is already reported in the unresolved thread at
rowid_fetcher.cpp:1001. - The regression metadata-oracle synchronization issue is resolved as deferred to follow-up, not fixed. The new regression suite was not independently executed here.
- The point-query test formatting thread remains unresolved, and the live
Clang Formattercheck is failing on this exact head. - The commit-TSO metadata-loss and ordered linked-compaction defects remain pre-existing producer-side issues tracked in their existing threads.
Critical checkpoints:
- Goal, approach, scope, and result correctness: the PR consistently resolves singleton published commit TSO through a scan-local logical reader while preserving physical reads for unpublished and multi-version rowsets. All changed files are relevant to that boundary, its consumers, cache behavior, or coverage.
- Parallel paths and lifecycle: ordinary scans, page/segment ZoneMaps, residual predicates, statistics, inverted/ANN/bloom/dictionary fallbacks, point queries, row-id materialization, and historical reads keep the owning rowset id/version/TSO and source-rowset lifetime. No additional bypass was found.
- Concurrency and locking: duplicate
ColumnReaderCacheinsertion is serialized under the existing mutex and preserves one map/list entry, ownership, size accounting, and eviction behavior. - Compatibility, persistence, configuration, and data writes: this change adds no protocol, storage-format, configuration, FE/BE variable, or new write-path contract. The known producer-side metadata defects are outside the changed reader implementation and remain tracked separately.
- Observability and performance: point-query reads now reuse the normal statistics, I/O context, and cache-write limiter. The only remaining new-path performance concern found is the already-threaded TSO-only JSONB work.
- Tests and validation: test intent and assertions cover the important singleton/multi-version, cold/warm cache, projection, row-store, delete-sign, limiter, and historical-read cases. This review was static only: I did not build or run tests. The author reports 85 ASAN tests passing; the SQL regression suite was not run locally, no live-cloud integration/performance benchmark is reported, and the current formatter check fails.
Completion: converged in Round 1 of 3 with no new valuable finding.
…cuit ### What problem does this PR solve? Issue Number: None Related PR: apache#65684, apache#68097 Problem Summary: A singleton rowset stores only a placeholder for `__DORIS_VERSION_COL__` and `__DORIS_COMMIT_TSO_COL__`; the real value comes from the rowset metadata at read time. The short-circuit point query reads the JSONB row store, which holds that placeholder, and the BE fast path deliberately does not resolve these columns (see `point_query_executor.cpp`). So on a MOW table with `store_row_column = true`: ```sql CREATE TABLE b (id INT NOT NULL, v INT) UNIQUE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES ('replication_num'='1', 'enable_unique_key_merge_on_write'='true', 'store_row_column'='true', 'changelog.enable'='true'); INSERT INTO b VALUES (1, 7), (2, 8); SELECT __DORIS_VERSION_COL__, __DORIS_COMMIT_TSO_COL__, v FROM b WHERE id = 1; -- before: 0, 0, 7 after: 2, <commit tso>, 7 ``` The columns can be named explicitly without `show_hidden_columns`, and `SELECT *` returns them once `show_hidden_columns` is on. The regular scan path resolves them from the rowset correctly. `LogicalResultSinkToShortCircuitPointQuery` now keeps any query that reads a hidden column on the regular scan: it checks the project's input slots for `sink(project(filter(scan)))` and the sink's input slots for `sink(filter(scan))`. Queries reading only visible columns keep the short circuit, including a plain `SELECT *` (hidden columns are not expanded by default) and server-side prepared statements. ### Release note Fix point queries on row-store MOW tables returning 0 for `__DORIS_VERSION_COL__` and `__DORIS_COMMIT_TSO_COL__`. Point queries that read hidden columns no longer use the short-circuit path. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - FE UT `ShortCircuitPointQueryTest` (10 cases, 2 new) passes. - New regression test `test_short_circuit_rowset_hidden_columns` fails on the unfixed FE (explain still shows SHORT-CIRCUIT) and passes with the fix; all 20 suites under `point_query_p0` pass. - Manual, local coupled cluster: the point query returns version 2 and the real commit TSO (default short circuit, `enable_short_circuit_query_access_column_store=false`, and server-side prepared statements); prepared statements reading only visible columns, and a plain `SELECT *`, still take the short circuit. - Behavior changed: Yes. Point queries that read a hidden column (explicitly, or through `SELECT *` with `show_hidden_columns = true`) run as a regular scan instead of the short circuit. With `show_hidden_columns = true` such a query now also returns a row deleted by the delete sign, as a regular scan already does. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
What problem does this PR solve?
Issue Number: None
Related PR: #68018
Problem Summary:
A single-version rowset stores a placeholder in
__DORIS_COMMIT_TSO_COL__; its logical value comes from the rowset's publish metadata. Some read paths request a physical column reader before the data iterator supplies that value. A cross-column condition such ascommit_tso > 0 OR v = 99can therefore prune a segment using the placeholder ZoneMap. Historical reads and JSONB-based row-id/point reads can also return the placeholder.Resolve the logical reader in
Segment::get_column_readerusing the source rowset's version and commit TSO, before consulting physical metadata or the cache. Single-version published reads receive a constant reader; multi-version and unpublished internal reads retain physical readers. Keep only physical readers inColumnReaderCache, removing the optional constant parameter and cache-upgrade state. Rename deliberate Variant/storage metadata accesses toget_physical_column_reader.Route segment/page expression ZoneMaps, ordinary predicates, aggregate statistics, data iterators and indexes through this entry point. Predicate simplification uses the reader's logical ZoneMap and reuses the first-phase TSO reader, including with a cold physical-reader cache; existing expression dependencies still determine whether TSO needs materialization. Supply source rowset metadata to historical and row-id reads.
For point queries, always treat TSO as unavailable from JSONB and read it through the existing missing-column loop. Explicit row-store projections prevent decoding or default-filling the TSO placeholder; empty projections skip JSONB reads. Reuse the pinned source rowset, query I/O context, cache-write limiter and profile statistics for TSO and other missing columns. Ordinary missing columns remain subject to the column-store access setting, while required TSO resolution remains available when it is disabled. Point queries requesting TSO bypass the JSONB row cache because it does not retain the source rowset location. Historical JSONB and two-phase reads also resolve TSO using source rowset metadata.
This is an alternative to #68018's cache-upgrade approach. It retains that PR's same-key LRU replacement fix and constant-reader index no-op: concurrent physical cache misses can still insert the same key, and physical indexes are inapplicable to logical constants. No storage format or FE/BE protocol changes.
Release note
Fix incorrect commit TSO values and filtering in expression pushdown, historical reads, two-phase row-id fetch and point queries. Preserve point-query file-cache write limits and detailed I/O statistics when reading commit TSO.
Check List (For Author)
tso_p0/test_commit_tso_reader_pathsuses runtime rowset metadata as the oracle and includes TSO-only/mixed row-store projections with ordinary column-store access disabled; not executed because this worktree has no independent FE/BE cluster.run-be-ut.sh -j 96:PointQueryCommitTsoTest,RowCacheTest,LookupConnectionCacheTest,ConstantColumnIteratorTest,ColumnReaderCacheTest,SegmentIteratorExprZonemapTest,HistoricalRowRetrieverTest, andRowIdStorageReaderTest. Physical multi-version TSO tests use the realCachedRemoteFileReaderwith a local object-storage stand-in and cover zero/exhausted cache-write budgets, an unlimited control, and query I/O statistics. No live cloud integration test or performance benchmark was run.git diff --checkpassed. Changed BE sources and the UT executable compiled and linked. clang-tidy was rerun; the existing unmatchedNOLINTENDinbe/src/core/types.h:576still blocks a clean result, so clang-tidy is not reported as passed. Earlier fullbuild.shreached a linkeddoris_bebut was blocked by an ASAN double-free in the kuromoji dictionary generator; that full build was not repeated for this update.Check List (For Reviewer who merge this PR)