Doris-Breakwater commented on issue #67995:
URL: https://github.com/apache/doris/issues/67995#issuecomment-5675539557
## 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 #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 #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
--
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]
