LuciferYang opened a new issue, #67995:
URL: https://github.com/apache/doris/issues/67995
### Search before asking
- [X] I had searched in the issues and found no similar issues.
### Version
master, 846d9b2ebfc.
### What's Wrong?
Three hidden columns store a placeholder on disk and get their real value
substituted at read time, so their zone maps describe the placeholder rather
than the values rows come back with. Zone-map pruning that trusts those
summaries drops rows that match.
| column | on disk | substituted at read time by |
|---|---|---|
| `__DORIS_BINLOG_TSO__` | all NULL (`ColumnNullable::insert_many_defaults`,
`be/src/storage/transform/row_binlog_derive.cpp:126-129`) |
`SegmentIterator::_update_tso_col_if_needed`,
`be/src/storage/segment/segment_iterator.cpp:2476` |
| `__DORIS_COMMIT_TSO_COL__` | `0` | a `ConstantColumnReader` built in
`Segment::new_iterator` / `new_column_iterator`,
`be/src/storage/segment/segment.cpp:422-437`, `:947-966` |
| `__DORIS_VERSION_COL__` | `0` (default `ZERO_NUMBER`) |
`SegmentIterator::_replace_version_col_if_needed`,
`be/src/storage/segment/segment_iterator.cpp:2449` |
The `ColumnPredicate` paths already handle the two TSO columns.
`Segment::new_iterator` synthesises `{min = max = commit_tso, has_not_null =
true}` for the binlog TSO column and prunes against the real value
(`be/src/storage/segment/segment.cpp:449-465`), the page-level loop skips it
(`be/src/storage/segment/segment_iterator.cpp:1153`), and the commit TSO column
is covered by the constant reader above.
The expression zone-map paths have neither guard.
`build_segment_zonemap_context` (`be/src/storage/segment/segment.cpp:94-145`)
and the page-level expression loop
(`be/src/storage/segment/segment_iterator.cpp:3406-3415`) check only
`Segment::can_apply_predicate_safely`, which is about variant target types and
returns true here.
For the binlog TSO column the failure is stronger than a wrong bound. Its
on-disk zone map has `has_not_null = false`, and every evaluator treats that as
"the comparison is NULL on every row, so no row can be TRUE" and returns
`kNoMatch` before looking at any bound:
`be/src/exprs/function/functions_comparison.h:337-338`, `:380-382`, and
`be/src/exprs/expr_zonemap_filter.cpp:498` for `IS NOT NULL`. So one comparison
on that column prunes the whole single-version binlog segment, for any operator
and any other operand, even though after substitution the rows carry a non-null
TSO.
`__DORIS_VERSION_COL__` has no guard on any path, including
`ColumnPredicate`, and it is reachable from plain SQL:
- It exists on effectively every unique-key table: added when
`Config.enable_hidden_version_column_by_default && keysType == UNIQUE_KEYS &&
!hasSeqMapping`
(`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java:1070-1076`),
and that config defaults to true
(`fe/fe-common/src/main/java/org/apache/doris/common/Config.java:1500`).
- It is invisible, but invisibility only suppresses star expansion
(`fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/ExpressionAnalyzer.java:436-441`);
binding an explicitly named slot has no visibility check, and it is in the
scan output (`LogicalOlapScan.computeOutput` builds from `getBaseSchema(true)`,
`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOlapScan.java:826`).
Existing regression cases already name it directly.
- On merge-on-write its aggregation is `NONE` (`CreateTableInfo.java:1071`),
so a predicate on it lands in the storage-layer set rather than the post-merge
one (`TabletReader::_init_column_predicates`,
`be/src/storage/tablet/tablet_reader.cpp:389-402`) and reaches zone-map
pruning. On merge-on-read it is `REPLACE` and stays above the merge.
- The substitution happens before predicate evaluation, so the two disagree:
`_read_columns_by_index` then `_replace_version_col_if_needed` then
`_evaluate_vectorization_predicate`
(`be/src/storage/segment/segment_iterator.cpp:2959`, `:2960`, `:2978`). Row
evaluation sees the real version while the zone map says `[0, 0]`.
So on a unique merge-on-write table, `WHERE __DORIS_VERSION_COL__ = <n>`
with `n != 0` returns nothing on single-version rowsets, while the rows it asks
for exist and row-level evaluation would have matched them.
### What You Expected?
A column whose stored value is a placeholder either does not drive zone-map
pruning, or drives it through a summary of the value rows will actually have.
Pruning and row-level evaluation should not disagree about the same predicate.
### How to Reproduce?
The version column case is plain SQL on a unique merge-on-write table with
`n != 0`, comparing results with pruning on and off:
```sql
CREATE TABLE t (k INT, v INT) UNIQUE KEY(k) DISTRIBUTED BY HASH(k) BUCKETS 1
PROPERTIES ("replication_num" = "1", "enable_unique_key_merge_on_write" =
"true");
INSERT INTO t VALUES (1, 1);
INSERT INTO t VALUES (2, 2);
SELECT k, __DORIS_VERSION_COL__ FROM t; -- shows the real
versions
SELECT count(*) FROM t WHERE __DORIS_VERSION_COL__ = <a version from above>;
```
The two TSO columns need a pushed common expression on them rather than a
`ColumnPredicate`; `IS NULL` / `IS NOT NULL` qualify today
(`be/src/exprs/function/is_null.h:67`,
`be/src/exprs/function/is_not_null.h:69`), and column-vs-column comparison
would once #67774 lands, which is what surfaced this.
I have not run the SQL above; it is derived from the code path, and the
aggregation-type split means it should reproduce on merge-on-write and not on
merge-on-read.
### Anything Else?
The substituted values are known exactly where the builders run, so the fix
can synthesise a real summary rather than only skip: `commit_tso.end_tso()` (or
`0` when it is `-1`) for the TSO columns, `version.second` for the version
column, both always non-null. Note that the comment at
`be/src/storage/segment/segment_iterator.cpp:2478` claims the physical-time
part of the commit TSO is used; the code inserts the raw value (`:2493`,
`:2500-2503`, `:2511-2513`, `:2525`), matching the formula the
`ColumnPredicate` path already uses at
`be/src/storage/segment/segment.cpp:456-461`. The comment is stale and
misleading.
Routing the binlog TSO column through the constant-reader cache instead
would be wrong: its substitution only happens for `read_row_binlog` reads
(`be/src/storage/segment/segment_iterator.cpp:2483-2485`), and a cached
constant reader would then be served to non-binlog reads of the same segment.
That cache has a separate problem, filed as #67994.
### Are you willing to submit PR?
- [X] Yes I am willing to submit a PR!
### Code of Conduct
- [X] I agree to follow this project's [Code of
Conduct](https://www.apache.org/foundation/policies/conduct)
--
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]
