Doris-Breakwater commented on issue #68240: URL: https://github.com/apache/doris/issues/68240#issuecomment-5747499964
Breakwater-GitHub-Analysis-Slot: slot_f821e4f614d8 ### Initial assessment This is a credible **data-correctness bug on master**, not just a performance/optimization issue. Static inspection of the cited baseline `846d9b2ebfc595ddfd77579447db16a674c901f7` confirms the underlying contract mismatch: VERSION, COMMIT_TSO, and row-binlog BINLOG_TSO may be logical rowset/context-derived values, while some consumers still see physical segment placeholders. That can produce both wrong projected values and false-negative pruning. The issue currently has no labels, assignee, milestone, or linked development item; it should be linked to #68125 and labeled as a storage/read-path data-correctness bug according to the repository's label taxonomy. ### Code-confirmed facts - The normal `SegmentIterator` path explicitly replaces a singleton rowset's physical VERSION placeholder and performs BINLOG_TSO replacement only when `read_row_binlog` is set. This proves that the physical value and the logical scan value intentionally differ in those contexts. - At the cited baseline, segment/page predicate pruning and expression ZoneMap evaluation can obtain summaries from the physical `ColumnReader`. The COMMIT_TSO constant-reader support is also vulnerable to cache order because `ColumnReaderCache` is keyed by column UID/path and returns a cached reader before considering the requested constant. - `Segment::seek_and_read_by_rowid` only reads the selected physical column through a `ColumnIterator`; it has no post-read VERSION/COMMIT_TSO replacement. Both short-circuit point queries and lazy two-phase row-ID fetches call this entry point with read options that do not carry the owning rowset's logical value. - The point-query full-row-store path decodes JSONB directly. A row-cache hit is keyed by tablet/key and bypasses row-location/rowset lookup, so that cached payload cannot independently derive the owning rowset's VERSION or COMMIT_TSO. - The lazy row-ID path does retain the exact `BetaRowset`, which is sufficient to repair the result per batch, but the baseline does not use that metadata after either JSONB decoding or column reads. - MIN/MAX pushdown can use the statistics iterator and physical ZoneMaps instead of normal scan materialization. VERSION therefore needs a normal-iteration fallback unless the statistics reader can expose the logical singleton; COMMIT_TSO can retain the fast path only when its constant reader is correct and isolated from the physical-reader cache. These facts explain the reported execution-path and cache-warmup dependence. They also show why an unconditional replacement would be wrong: multi-version rowsets, unassigned COMMIT_TSO, intentionally physical/internal reads, and BINLOG_TSO outside a row-binlog read do not share the same semantics. ### Assessment of the proposed fix At the inspected and current #68125 head, `0022410d4b3cddbb48c548c47bc22b4a7e99d5d8`, the design is directionally consistent with the required contract: - hidden-column identification and value resolution are centralized; - segment predicates/common-expression ZoneMaps use a synthetic logical singleton; - physical page ZoneMap, Bloom-filter, and inverted-index pruning are bypassed when a read-time constant applies; - context-dependent constant readers do not enter the UID-only physical reader cache; - MIN/MAX falls back to `SegmentIterator` for logical constants not supported by the statistics reader, while COMMIT_TSO retains its constant-reader fast path; - point reads resolve the owning rowset, avoid context-free row-cache hits when a read-time hidden column is projected, and replace only the newly decoded row; - two-phase row-ID reads replace only the newly appended suffix using the exact batch rowset; and - direct reads intentionally do not synthesize BINLOG_TSO, leaving that context-dependent behavior in `SegmentIterator`. This is a static assessment, not runtime verification. I did not build Doris or run a cluster/test suite during this triage. The issue itself also states that its reproduction was not rerun. ### Missing evidence / validation gaps Before closing this as fixed, please attach results from an affected baseline and from the proposed fix head: 1. Exact outputs for the two point queries, plus `EXPLAIN` or profile evidence that the first query used the short-circuit path. Use the source rowset version as the expected-value oracle rather than only asserting `> 0`. 2. The same point test for full and partial row storage, both with a cold row cache and after an ordinary-column point query warms it. 3. Lazy TopN plan output containing `PhysicalLazyMaterialize`, with interleaved singleton rowsets and exact per-row VERSION expectations for both column-store and full-row-store fetches. 4. COMMIT_TSO projection and predicates with the owning rowset metadata as the oracle, including direct reads and both physical-reader-first and logical-reader-first cache order. 5. Segment- and page-level ordinary predicates/common expressions with cold and warm caches, and explicit coverage of ZoneMap, Bloom-filter, and inverted-index behavior. 6. MIN/MAX plan/profile evidence proving which queries use `VStatisticsIterator` and which deliberately fall back; result equality alone does not validate the optimization path. 7. Actual row-binlog BINLOG_TSO coverage, plus negative controls for multi-version rowsets, unpublished/unassigned COMMIT_TSO, non-row-binlog reads, and intentionally physical internal reads. The added tests in #68125 cover many important mechanics, including exact `2,3,2,3` lazy-fetch VERSION values, both reader-cache orders, VERSION statistics fallback, and COMMIT_TSO statistics use. The main remaining acceptance gaps are end-to-end path proof for short-circuit and MIN/MAX, row-cache warmup, direct COMMIT_TSO, and the positive/negative row-binlog context matrix. ### Recommended next step Use #68125 as the primary fix, run its targeted BE test (`SegmentIteratorExprZonemapTest.*`) and point-query regression on both the baseline and PR head, then add the missing assertions above before merge. If those tests pass, this issue can serve as the umbrella tracker for #67994/#67995 and the direct-read/statistics extensions; no separate speculative fix is warranted. If any path still disagrees, capture the query profile, exact commit SHA, table properties, rowset version/commit TSO metadata, cache warmup order, and storage mode before assigning a new root cause. -- 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]
