[Bug] Zone-map pruning trusts placeholder summaries for the hidden TSO and version columns, dropping matching rows #67995

Activity

1. Doris-Breakwater commented on Sep 15, 2026

   

   Doris-Breakwater

   on Sep 15, 2026

   Collaborator

   More actions

   Initial maintainer analysis

   Triage: This is a high-confidence correctness issue from static analysis of the reported snapshot 846d9b2ebfc595ddfd77579447db16a674c901f7. It is not merely a missed optimization: a physical zone map is being used to decide the truth of predicates over a different, read-time value. The issue currently has no labels or assignee; the appropriate classification is the repository's bug/correctness label plus the BE storage/segment (zone-map) area.

   Verified facts

   • Expression zone-map pruning is enabled by default (SessionVariable.enableExprZonemapFilter = true; the BE helper also treats an unset query option as enabled).
   • build_segment_zonemap_context reads the physical ColumnReader summary, and SegmentIterator::_apply_expr_zonemap_to_row_ranges does the same for page summaries. Neither path recognizes read-time-substituted columns. Their only safety gate is Segment::can_apply_predicate_safely, whose implementation only handles variant target-type compatibility and returns true for these BIGINT columns.
   • The reported placeholder/substitution mismatch is present:
     • __DORIS_BINLOG_TSO__ is written as NULL by fill_binlog_system_columns, then replaced with a non-NULL commit_tso.end_tso() (or 0 for -1) for single-version read_row_binlog reads.
     • __DORIS_VERSION_COL__ has default 0, then SegmentIterator::_replace_version_col_if_needed replaces it with version.second for single-version rowsets.
     • __DORIS_COMMIT_TSO_COL__ has default 0; the intended read path requests a ConstantColumnReader carrying the assigned commit TSO for a single-version rowset.
   • Row-level evaluation occurs after _replace_version_col_if_needed and _update_tso_col_if_needed, while segment/page zone-map pruning occurs before those substitutions. This establishes the semantic disagreement in the code path.
   • An all-NULL binlog-TSO zone map makes comparison evaluators return kNoMatch immediately and makes IS NOT NULL return kNoMatch, so an expression over the read-time non-NULL value can eliminate a whole segment/page.
   • The version-column reachability analysis is also supported by the snapshot: the hidden column is enabled by default for UNIQUE KEY tables without sequence mapping, uses aggregation NONE for merge-on-write and REPLACE for merge-on-read, is included in LogicalOlapScan output via getBaseSchema(true), and explicit slot binding does not reject an invisible column. Existing regression suites explicitly reference this column.

   There is an additional wrong-result mode beyond the false negatives described in the issue. After creating the iterator, Segment::new_iterator calls ColumnReader::prune_predicates_by_zone_map on cached readers without a placeholder-column guard. For a single-version rowset whose semantic version is positive, the physical [0, 0] summary can mark a range predicate such as __DORIS_VERSION_COL__ < 1 as always true; the predicate is then removed even though it is false for the substituted value. That can admit nonmatching rows. The fix and tests should therefore cover both “no-match” pruning and “always-true” predicate removal.

   Scope and related issue

   • The unsafe synthesized-value cases are limited by the same guards as their substitutions: single-version rowsets for version/commit TSO, and additionally read_row_binlog for binlog TSO. Range/compaction rowsets and non-binlog reads of __DORIS_BINLOG_TSO__ must continue using physical values.
   • The commit-TSO expression path is entangled with [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 but this issue is not a duplicate. The expression builder can cache a non-constant reader before new_column_iterator requests the constant, while [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 shows that the cache key ignores the requested constant. Fixing only the cache does not make the binlog-TSO or version physical zone maps safe; fixing only the zone-map builders does not repair a previously cached wrong reader.
   • A nearby path should be audited before closing this issue: segment_zone_maps_can_answer_agg excludes the commit-TSO ordinal but not the version/binlog-TSO ordinals, and VStatisticsIterator reads MIN/MAX from zone maps. I have not established FE reachability for a pushed MIN/MAX over these hidden columns, so this is a test requirement/hypothesis rather than a confirmed additional symptom.

   Missing runtime evidence

   The reporter explicitly states that the SQL was not run. Static evidence is strong enough to accept the issue, but the PR should include runtime confirmation with:

   1. The exact SQL output using concrete returned version values (replace the <a version from above> placeholder), SHOW CREATE TABLE/full schema output, and the exact build SHA.
   2. EXPLAIN VERBOSE proving whether each predicate is a ColumnPredicate or a pushed common expression, plus a query profile containing RowsStatsFiltered, ExprZoneMapFilteredSegments, and ExprZoneMapFilteredPages where applicable.
   3. Rowset/segment topology proving the tested data remains in single-version rowsets (avoid a compaction race), and a range/compacted-rowset control case.
   4. For the TSO cases, the exact row-binlog read entry point, read_row_binlog state, assigned commit TSO, pushed expression shape, and results with enable_expr_zonemap_filter both on and off.

   Note that SET enable_expr_zonemap_filter = false is only a partial diagnostic/workaround: it disables the expression path, not the version column's ColumnPredicate zone-map path or the post-creation predicate-removal path.

   Recommended implementation and tests

   Centralize the concept of an effective read-time zone map for placeholder-backed columns, with the same conditions used by the substitution code. For an active substitution, expose a non-NULL degenerate summary (min = max = semantic value, has_not_null = true, has_null = false): version.second for the version column and the effective commit TSO (end_tso, or 0 where current read semantics use 0) for the TSO columns. Otherwise use the physical summary. This helper should be consumed consistently by:

   • segment-level expression zone-map evaluation;
   • page-level expression evaluation (the value is segment-constant, so every page has the same effective summary, or physical page pruning can be conservatively skipped);
   • segment/page ColumnPredicate pruning, including the existing binlog-TSO special case and the missing version case; and
   • the post-creation prune_predicates_by_zone_map optimization.

   Also audit statistics-iterator and any dictionary/bloom/index pruning of these columns so no other physical summary can make a semantic decision about a substituted value.

   Tests should include: version equality/range predicates that exercise both false-negative pruning and false-positive predicate removal; expression comparisons and IS NULL/IS NOT NULL; all three placeholder columns; segment and page paths; assigned and -1 TSO; read_row_binlog true/false; single-version versus range rowsets; merge-on-write versus merge-on-read; and cache call ordering coordinated with #67994. Each positive query should compare pruning enabled/disabled where that switch applies and assert returned rows, not only pruning counters.

   Breakwater-GitHub-Analysis-Slot: slot_1e4d5197b1b3
2. LuciferYang commented on Sep 15, 2026

   

   LuciferYang

   on Sep 15, 2026

   ContributorAuthor

   More actions

   Two things in that analysis are additions I had missed, and both change the scope of the fix.

   The always-true direction. ColumnReader::prune_predicates_by_zone_map runs on cached readers with no placeholder guard, so a physical [0, 0] summary can prove a range predicate such as __DORIS_VERSION_COL__ < 1 always true and remove it, admitting rows that do not match the substituted value. My report only described the false-negative direction. The fix has to cover predicate removal as well as pruning.

   The statistics path. segment_zone_maps_can_answer_agg excludes the commit-TSO ordinal but not the version or binlog-TSO ordinals, so a pushed min()/max() over those columns would answer from the physical summary. I have not established FE reachability for that either; I will check it before claiming it in a PR, and either fix it or say why it cannot be reached.

   Agreed on the shape of the fix: one effective-read-time-summary helper gated by exactly the substitution conditions (single-version for version and commit TSO, plus read_row_binlog for binlog TSO), consumed by segment and page expression evaluation, segment and page ColumnPredicate pruning including the existing binlog-TSO branch and the missing version case, and prune_predicates_by_zone_map. Patching the two expression builders alone, which is what I proposed, would leave the version column's ColumnPredicate path and the removal path wrong.

   On runtime evidence: the report is static, as it says. I have a local single-BE cluster and will attach the concrete SQL output, EXPLAIN VERBOSE showing which predicates are ColumnPredicate versus pushed common expressions, the profile counters, and a range-rowset control case with the PR rather than guess at them here.
3. LuciferYang commented on Sep 15, 2026

   

   LuciferYang

   on Sep 15, 2026

   ContributorAuthor

   More actions

   The statistics path is reachable for the version column, and not for binlog TSO. Traced, still static.

   max(__DORIS_VERSION_COL__) on a unique-key table returns 0 when enable_pushdown_minmax_on_unique is on (it defaults to false, SessionVariable.java:2379-2381). The chain: AggregateStrategies.pushdownMinMaxOnUniqueTable -> checkWhetherPushDownMinMax (fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java:487-528), which checks only that the original column exists and that its type is not complex/HLL/bitmap, with no visibility check and no synthetic-column check, unlike the generic path at :729-742; then TPushAggOp.MINMAX; then on BE use_statistics_iterator (be/src/storage/segment/segment.cpp:506-521) and segment_zone_maps_can_answer_agg, which skips only the commit-TSO ordinal and disqualifies only on pass_all, so a [0, 0] summary is judged usable; then VStatisticsIterator::next_batch (be/src/storage/iterator/vgeneric_iterators.cpp:63-93) and ColumnReader::next_batch_of_zone_map (be/src/storage/segment/column_reader.cpp:711-732), which report the placeholder. SegmentIterator::_replace_version_col_if_needed never runs on that path.

   __DORIS_BINLOG_TSO__ is safe here. It exists only in the row-binlog index schema (fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:2506), reachable only through RowBinlogTableWrapper, which reports DUP_KEYS, so the unique-table rule cannot fire; the generic rule then excludes it explicitly through RowBinlogTableWrapper.isRowBinlogSyntheticColumn (AggregateStrategies.java:729-734).

   One shape note for the fix: adding version_ordinal() next to the commit-TSO ordinal in segment_zone_maps_can_answer_agg would be wrong, because continue there means "do not disqualify this column". The gate has to set *usable = false when the read schema carries a substituted column, or produce the effective summary for it.

   Not verified: no cluster run of the SQL above, and the merge-on-read variant of the BE path is not fully traced.