Repository navigation
[fix](zonemap) Do not trust a cut string bound to prune or to answer MIN/MAX - #67642
Merged
Merged
Conversation
csun5285
requested review from
924060929,
airborne12,
eldenmoon,
englefly,
gavinchou,
liaoxin01,
morrySnow,
starocean999 and
yiguolei
as code owners
September 8, 2026 07:29
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
csun5285
force-pushed
the
fix/zonemap-cut-max-0xff
branch
from
September 8, 2026 11:21
0ad8ce8 to
5057549
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16826 ms |
Contributor
TPC-DS: Total hot run time: 81252 ms |
Contributor
ClickBench: Total hot run time: 14.64 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
yiguolei
previously approved these changes
Sep 9, 2026
morrySnow
requested changes
Sep 9, 2026
Comment on lines
+2383
to
+2385
| // Whether enable push down string type minmax to scan node. A string zone map bound is cut to | ||
| // 512 bytes, and the storage layer skips the push down for the segments whose bounds were | ||
| // actually cut, so turn this off only to stop pushing string minmax down at all. |
Contributor
There was a problem hiding this comment.
不能改变这个开关的语义，必须要强制走zonemap，否则统计信息收集就没法用了
Contributor
Author
There was a problem hiding this comment.
好，我把开关改成：是否强制使用 zonemap
csun5285
force-pushed
the
fix/zonemap-cut-max-0xff
branch
2 times, most recently
from
September 11, 2026 07:37
ec0e641 to
6d8df30
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16974 ms |
Contributor
TPC-DS: Total hot run time: 81382 ms |
Contributor
ClickBench: Total hot run time: 14.58 s |
yiguolei
reviewed
Sep 13, 2026
| // table. Every other query needs the real value. Without a runtime state, keep what the segment | ||
| // did before. | ||
| bool accepts_inexact_min_max(const StorageReadOptions& read_options) { | ||
| return read_options.runtime_state == nullptr || |
Contributor
There was a problem hiding this comment.
为什么 runtime_state == nullptr 意味着 inexact min max？
yiguolei
reviewed
Sep 13, 2026
| read_options.delete_condition_predicates->num_of_column_predicate() == 0; | ||
| bool use_statistics_iterator = | ||
| read_options.delete_condition_predicates->num_of_column_predicate() == 0 && | ||
| (delete_free || accepts_inexact_min_max(read_options)) && |
Contributor
There was a problem hiding this comment.
accepts_inexact_min_max 这个里面不是只控制了string 吗？
Contributor
Author
There was a problem hiding this comment.
哦对，这里应该改成不止是string，只要是统计信息的都应该让delete 失败，去读zonemap 的统计信息
csun5285
force-pushed
the
fix/zonemap-cut-max-0xff
branch
from
September 14, 2026 03:22
7a353d2 to
00ac6b6
Compare
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16945 ms |
Contributor
TPC-DS: Total hot run time: 83152 ms |
Contributor
ClickBench: Total hot run time: 14.84 s |
morrySnow
approved these changes
Sep 17, 2026
yiguolei
approved these changes
Sep 17, 2026
4 of 9 tasks
yiguolei
pushed a commit
that referenced
this pull request
Sep 22, 2026
…r to answer MIN/MAX (#67642) (#68324) ### What problem does this PR solve? Related PR: #67642 Backport #67642 to `branch-4.2`, cherry-picked from `b0f266a8ebc1a333f806831f5d2209f837fda5d5`. Fix string ZoneMap bounds at the 512-byte cutoff: carry when raising the maximum, discard an all-zero overflowed range on read, and fall back to reading rows for exact MIN/MAX when bounds were truncated. Preserve the original statistics-collection override and the old SQL variable name as an alias. ### Branch compatibility adaptations - `segment.cpp`: retain branch-4.2's `Schema` signature and column-ID iteration; use `schema.column(schema.column_id(i))` for the new string-type check. Add the original MIN/MAX forcing and fallback logic. - `SessionVariable.java`: use branch-4.2's `VariableMgr.VarAttr` with Chinese/English descriptions, preserving the new variable name, old alias, default, and Thrift mapping. - `PaloInternalService.thrift`: add only `force_pushdown_zonemap_minmax` with the original field ID `1006` and default `false`. Do not import the unrelated `1005` SNII option from master conflict context. - `vgeneric_iterators_test.cpp`: although this file auto-merged, adapt the added fixture to branch-4.2's `Schema`/`SchemaSPtr`, `SegmentWriterOptions`, `TestSegmentWriter`, and `finalize()` APIs. All original test cases remain. ### Validation - Verified all 12 original files are included; added/deleted lines in the other 9 files match the original patch. - No unresolved conflicts, conflict markers, or unstaged edits. Code whitespace check passed. The generated `.out` retains the original final blank separator, which default `git diff --check` reports as a blank line at EOF. - Confirmed the target Schema/writer/annotation APIs and checked that `TQueryOptions` field IDs are unique. - Regression suite and expected output retain the original content; all added BE test cases are retained. - Compilation, BE unit tests, and runtime regression tests were not run in this session; CI validation remains pending. ### Release note Fix incorrect string ZoneMap pruning and MIN/MAX results for truncated string bounds. ### Check List (For Author) - Test - [x] Regression tests included from #67642 (not run in this session) - [x] Unit tests included from #67642 with branch API adaptations (not run in this session) - [ ] Manual test - Behavior changed: - [x] Yes. Apply the ZoneMap and MIN/MAX correctness fixes from #67642. - Does this need documentation? - [x] No additional documentation changes for this backport. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label Co-authored-by: Chenyang Sun <[email protected]>
mrhhsg
added a commit
to mrhhsg/doris
that referenced
this pull request
Sep 22, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#68125, apache#68198, apache#67642 Problem Summary: Merge current master without restoring tablet schemas to the query read path. Identify constant readers from the segment schema and row-ID hidden columns from the per-slot fetch columns. Keep logical hidden-column MIN/MAX checks active in forced pushdown mode so VERSION and BINLOG_TSO do not return physical placeholders, while COMMIT_TSO retains its constant statistics reader and ordinary string bounds keep the existing approximation policy. ### Release note Preserve correct hidden-column reads and MIN/MAX results after the read-schema and forced ZoneMap pushdown changes. ### Check List (For Author) - Test: Unit Test / Manual test - Added forced and non-forced MIN/MAX coverage for VERSION, COMMIT_TSO and nullable BINLOG_TSO, including unassigned TSO. - clang-format v16, check-format, build hygiene and git diff --check passed. - ASAN BE build and FE build passed using build.sh. - Focused ASAN BE tests passed 35/35, covering hidden-column pruning/readers, string bounds and adjacent statistics iterators. - Restoring only the forced-pushdown bypass made the VERSION and BINLOG_TSO tests fail; restoring the fix passed all 35 tests again. - Point-query hidden-column/lazy-fetch and light-schema-change regression suites passed 2/2 on an isolated cluster; expected outputs were unchanged. - Added a specific test-fixture NOLINT for cognitive complexity inflated by GTest assertion macros. clang-tidy analysis remains blocked by the pre-existing unmatched NOLINTEND in be/src/core/types.h:576. - Behavior changed: Yes. Forced MIN/MAX cannot use physical placeholders for logical hidden-column values. - Does this need documentation: No
Merged
16 tasks
yiguolei
pushed a commit
that referenced
this pull request
Oct 13, 2026
…h a delete predicate (#68958) Introduced by #68324, the branch-4.2 pick of #67642: with `force_pushdown_zonemap_minmax` on, a segment that has a delete predicate is now answered by `VStatisticsIterator`. The read schema is implemented differently on the two branches. On master, the `ReadSchema` from #66472 keeps the delete predicate columns out of `num_block_columns()`, so `VStatisticsIterator` only builds iterators for the block columns. branch-4.2 has no such split: the schema ends with those delete predicate columns, `VStatisticsIterator` builds iterators for them too, and `DCHECK(block->columns() == _column_iterators.size())` aborts ASAN/debug BEs. ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> Co-authored-by: Claude Opus 5.5 <[email protected]>
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)