Doris-Breakwater commented on issue #67994: URL: https://github.com/apache/doris/issues/67994#issuecomment-5675536148
Breakwater-GitHub-Analysis-Slot: slot_d396904bac92 ## Initial maintainer analysis **Assessment:** this is a valid, high-confidence BE correctness defect at the reported `master` commit [`846d9b2ebfc595ddfd77579447db16a674c901f7`](https://github.com/apache/doris/commit/846d9b2ebfc595ddfd77579447db16a674c901f7). The cache-level contract violation is deterministic from source: a request carrying `const_value` can receive an already-cached physical reader. That can expose the stored `0` instead of the rowset `commit_tso`. The issue should be accepted as a storage/table-stream correctness bug; it is an in-memory read corruption risk, not evidence of on-disk corruption. The live issue currently has no labels, assignee, milestone, comments, or linked PR. `kind/bug` is appropriate; the repository's normal storage/table-stream area label should also be applied if one exists. ### Verified facts 1. [`ColumnReaderCache::get_column_reader`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/column_reader_cache.cpp#L96-L145) looks up only `(col_uid, empty_path)` and returns immediately on a hit. `const_value` reaches `ColumnReaderOptions` only on a miss. [`ColumnReader::create`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/column_reader.cpp#L495-L507) constructs a `ConstantColumnReader` only when that option survives to creation. Thus the proposed cache unit-test order (physical request, then constant request) necessarily returns the same physical reader today. 2. The reported ordering is correct. The segment expression-zone-map builder requests a reader without a constant ([builder call](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L94-L142)) and is run before the `SegmentIterator` is constructed ([`Segment::new_iterator`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L413-L520)). `SegmentIterator::init()` creates its column iterators only afterwards ([initialization](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment_iterator.cpp#L400-L478), [column-reader request](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment_iterator.cpp#L1720-L1758)). The page-zone-map path also makes a bare request ([source](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/stora ge/segment/segment_iterator.cpp#L3377-L3425)). 3. [`Segment::new_column_iterator`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L926-L987) derives the real constant for the commit-TSO UID on a single-version rowset. A successful constant reader creates a `ConstantColumnIterator` over that value ([source](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/column_reader.h#L1070-L1102)). If the cache returns the physical reader instead, this replacement is lost. The later `_update_tso_col_if_needed` safeguard is limited to `read_row_binlog` and the binlog TSO ordinal ([source](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment_iterator.cpp#L2476-L2493)); it is not a fallback for the commit-TSO constant-reader path. 4. Cross-query persistence is real but bounded. The reader cache is owned by `Segment`, while query reads use the segment LRU by default ([segment loading](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment_loader.cpp#L35-L87), [query default](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/rowset/beta_rowset_reader.cpp#L239-L248)). A bad entry can therefore affect later queries until that column-reader entry or the segment is evicted. It is not guaranteed to last for the entire segment-cache residency because the per-segment column cache has its own LRU limit. 5. Existing tests verify direct `ColumnReader::create` and constant-iterator behavior ([test](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/test/storage/segment/constant_column_iterator_test.cpp#L39-L74)), but [`column_reader_cache_test.cpp`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/test/storage/segment/column_reader_cache_test.cpp#L199-L230) does not cover changing from a physical request to a constant request for the same key. ### Reachability qualification and missing evidence The cache defect itself does not need logs or a Doris profile to confirm. The exact current end-to-end trigger and user-visible blast radius are not yet demonstrated, however: - Simple `IS NULL` / `IS NOT NULL` expressions are normally normalized into `ColumnPredicate`s ([source](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/exec/operator/scan_operator.cpp#L967-L1011)), and the built-in time-travel filter is also a column-vs-literal comparison. Those ordinary forms install/request the constant in the earlier predicate loop unless the segment was already warmed with a physical reader. - The null functions are zone-map evaluable, but that alone does not prove the issue's required condition that the expression remains in `common_expr_ctxs_push_down` with no same-column `ColumnPredicate`. A compound expression can plausibly have that shape, but no SQL or serialized plan was supplied. - PR #67774 is still open, so its column-vs-column trigger is future exposure rather than a current merged path. For an end-to-end confirmation, please add: table DDL and binlog/time-travel properties; the exact SQL sequence (including the cache-warming query); `EXPLAIN VERBOSE`; confirmation that the affected rowset is single-version with a nonzero `commit_tso`; and, if available, a query profile showing expression-zone-map pushdown/counters. This evidence is useful for impact qualification, not a prerequisite for fixing the deterministic cache bug. ### Required fix properties and tests 1. A request with `const_value` must be authoritative even when the UID is already cached. Upgrade/replace the cached physical reader atomically and return the constant reader. Because the constant is segment/rowset intrinsic, later bare requests should not downgrade it. 2. Do not implement replacement by merely calling the existing `_insert_locked_nocheck` for an existing key: it appends a second LRU node and overwrites only the map iterator, leaving stale LRU/accounting state. Add a true same-key upsert/replace operation under `_cache_mutex`, and define the concurrent physical-vs-constant winner explicitly. 3. A cache-only upgrade fixes subsequent data reads, but it cannot retroactively correct a first expression-zone-map evaluation that already consumed the physical `[0,0]`. The segment- and page-zone-map builders must request the same constant, or constant-reader selection must be centralized in an API where those callers cannot omit the rowset constant. Bare internal readers are why the cache-level upgrade is still necessary. 4. Add cache unit tests for physical-then-constant, constant-then-bare, repeated same-key replacement (cache size remains one), and concurrent physical/constant requests. Validate both iterator output and the constant zone map, not only the concrete reader type. 5. Add a segment-level regression that starts with a zone-map-evaluable common expression and no same-column `ColumnPredicate`, proves the first zone-map evaluation uses the real `commit_tso`, and then proves row reads return the same nonzero value. Add a second-query/cache-reuse case. If #67774 lands, include its column-vs-column shape as an additional 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]
