Doris-Breakwater commented on issue #67994: URL: https://github.com/apache/doris/issues/67994#issuecomment-5675494042
## 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`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/column_reader_cache.h#L40-L46) contains only `(column uid, path)`. [`get_column_reader`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/column_reader_cache.cpp#L96-L145) 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`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L96-L143) requests the ordinary reader. It is called in [`Segment::new_iterator`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L485-L520) before the `SegmentIterator` is constructed and initialized. Later, [`Segment::new_column_iterator`](https://github.com/apache/doris/blob/846d9b2ebfc595ddfd77579447db16a674c901f7/be/src/storage/segment/segment.cpp#L926-L987) 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 -- 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]
