[Bug] The column reader cache drops a requested constant value on a cache hit, so a placeholder column can be read as its on-disk placeholder #67994

Activity

1. Doris-Breakwater commented on Sep 15, 2026

   

   Doris-Breakwater

   on Sep 15, 2026

   Collaborator

   More actions

   Breakwater initial analysis

   Verdict: Confirmed code-level correctness defect on master at 846d9b2ebfc595ddfd77579447db16a674c901f7. The cache contract is inconsistent with the new const_value input: a cold lookup uses it to create a different reader type, while a hit returns the existing reader without considering it. This is sufficient to establish the root cause statically. The exact SQL-level trigger and production frequency are not yet demonstrated, so those parts of the impact remain unverified.

   Verified facts

   • ColumnReaderCacheKey contains only (column uid, path). get_column_reader performs that lookup before inspecting const_value; only the miss path forwards const_value to ColumnReader::create, which constructs a ConstantColumnReader.
   • build_segment_zonemap_context requests the ordinary reader. It is called in Segment::new_iterator before the SegmentIterator is constructed and initialized. Later, Segment::new_column_iterator supplies the rowset commit_tso, but will receive the already-cached on-disk reader. The page-level expression zone-map path also requests the ordinary reader.
   • The on-disk default is 0, and the existing segment test explicitly writes 0 for this hidden column. ConstantColumnReader::new_iterator is the component that substitutes the read-time value. Therefore, after the ordinary reader wins the cache entry, later data reads use the on-disk iterator and can emit 0; this is not confined to pruning.
   • Query scans use the segment cache by default, and the ColumnReaderCache is owned by the cached Segment, so a contaminated entry can survive the initiating query until that segment is evicted. This is read-time result corruption; there is no evidence here of persisted data corruption.
   • Existing tests cover a cold-cache new_column_iterator constant read and a ColumnPredicate pruning path, but do not cover an ordinary-reader cache hit followed by a constant request. IS NULL/IS NOT NULL are zone-map-evaluable and can prime the expression path, although an end-to-end plan proving access to this hidden column has not been supplied.
   • The issue currently has no labels. The related column-vs-column expression zone-map PR is still open and would widen the trigger surface if merged without addressing this cache invariant.

   Assessment / priority

   This should be triaged as a BE storage correctness bug, with high priority before merging the related expression-zone-map expansion. The trigger is conditional, but the failure is deterministic once a non-constant caller populates the per-segment entry first. Disabling expression zone-map filtering avoids the demonstrated priming path, and disabling segment caching limits cross-query persistence, but neither is a complete correctness fix because other non-constant callers exist and the within-query ordering remains possible.

   Information still needed to quantify user-visible impact

   • A minimal SQL/DDL sequence that exposes the hidden commit-TSO column through a pushed common expression, plus EXPLAIN VERBOSE.
   • The relevant query profile, especially the pushed predicates/common expressions and ExprZoneMapFilteredSegments, ExprZoneMapFilteredPages, and ExprZoneMapUnusableEvals counters.
   • Values of enable_expr_zonemap_filter and enable_segment_cache, the expected and actual rows, and whether repeating the query or disabling the segment cache changes the result.

   Recommended next steps

   1. Fix the cache centrally so const_value participates in lookup semantics (for example, a separate constant-reader key/path or a non-cached constant reader after preserving column-existence validation). Updating only the two expression zone-map callers is insufficient.
   2. Do not simply ignore the hit and insert a second reader under the same current key: _insert_locked_nocheck does not replace/remove an existing LRU node, so that approach can leave duplicate list nodes and inconsistent map/accounting state.
   3. Add a focused ColumnReaderCache test for ordinary request -> same uid with const_value, and assert both the reader type and values returned by its iterator. Also extend the existing commit-TSO segment test to prime the ordinary reader before new_column_iterator, then verify that the real commit_tso, not 0, is read.
   4. Add reverse-order/repeated-hit and mixed concurrent lookup coverage, plus an end-to-end regression once a stable SQL trigger is available. Verify eviction/accounting invariants for whichever cache design is chosen.

   The reporter is willing to submit a PR; the proposed cache-level scope is appropriate.

   Breakwater-GitHub-Analysis-Slot: slot_d396904bac92
2. Doris-Breakwater commented on Sep 15, 2026

   

   Doris-Breakwater

   on Sep 15, 2026

   Collaborator

   More actions

   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. 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 looks up only (col_uid, empty_path) and returns immediately on a hit. const_value reaches ColumnReaderOptions only on a miss. ColumnReader::create 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) and is run before the SegmentIterator is constructed (Segment::new_iterator). SegmentIterator::init() creates its column iterators only afterwards (initialization, column-reader request). The page-zone-map path also makes a bare request (source).

   3. Segment::new_column_iterator 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). 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); 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, query default). 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), but column_reader_cache_test.cpp 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 ColumnPredicates (source), 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 [Enhancement](zonemap) Support column-vs-column comparisons in expression zone map pruning #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 [Enhancement](zonemap) Support column-vs-column comparisons in expression zone map pruning #67774 lands, include its column-vs-column shape as an additional regression.
3. LuciferYang commented on Sep 15, 2026

   

   LuciferYang

   on Sep 15, 2026

   ContributorAuthor

   More actions

   Correcting my own trigger claim, which the analysis above is right to qualify.

   I wrote that IS NULL / IS NOT NULL already qualify as pushed common expressions. Zone-map evaluability was the wrong thing to check. ScanLocalStateBase::_normalize_is_null_predicate (be/src/exec/operator/scan_operator.cpp:967-1011) converts is_null_pred into a column range, so such a predicate normally leaves the common-expression set and arrives as a ColumnPredicate, which makes the earlier predicate loop install the constant reader first. So no currently merged shape is demonstrated to reach this; the reachable one is the column-vs-column comparison from #67774, which can never become a ColumnPredicate.

   Two other corrections to my wording: the bad entry lasts until the per-segment column cache evicts it or the segment is evicted, not necessarily the whole segment residency; and the defect is the dropped constant on the lookup, independent of any trigger.

   On the fix: the same-key replacement trap named in point 2 is real. _insert_locked_nocheck pushes a second LRU node and only overwrites the map iterator, and eviction erases _cache_map[tail->key], which can drop the live entry of that key. My patch makes it a same-key in-place update instead of a second insert. I will extend the unit tests to assert the constant reader's zone map and iterator output rather than only its type, and add the concurrent physical-versus-constant case.

   Point 3 is the reason #67995 exists separately: a cache upgrade cannot repair a zone-map evaluation that already consumed the physical [0,0], so the builders have to request the constant themselves.
4. LuciferYang commented on Sep 15, 2026

   

   LuciferYang

   on Sep 15, 2026

   ContributorAuthor

   More actions

   One more piece of evidence, which I should have found before filing: the hazard is already known and worked around at another call site.

   segment_zone_maps_can_answer_agg walks every column and requests a reader without a constant, and it skips exactly one ordinal for exactly this reason (be/src/storage/segment/segment.cpp:148-160):

           // 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.
           if (static_cast<int32_t>(ordinal) == schema.commit_tso_ordinal()) {
               continue;
           }

   So "a bare request for that column poisons the cache for every later read" is not my inference; it is the documented reason a loop three functions above the zone-map builders avoids the column. The two expression zone-map builders make the same bare request and do not skip it.

   That also means the fix at the cache level makes this call site's skip unnecessary rather than wrong, so I will leave it alone in the cache PR and let #67995 decide what those loops should do with placeholder columns.
5. LuciferYang commented on Sep 15, 2026

   

   LuciferYang

   on Sep 15, 2026

   ContributorAuthor

   More actions

   Opened #68018.