[fix](zonemap) Treat reversed zone map bounds as invalid by csun5285 · Pull Request #67431 · apache/doris

csun5285 · 2026-09-02T06:22:23Z

NaN and infinity are recorded in the has_nan/has_positive_inf/ has_negative_inf flags and never move min/max, so a page holding nothing else leaves both at the values add_values() starts from: min = DBL_MAX and max = -DBL_MAX. Those are written out as if they were data, and the flags stand in for at most one side, so reading them back reports a DBL_MAX that is nowhere in the page -- SELECT MIN(v) answers with it while SELECT COUNT(*) WHERE v = that value answers 0. On data written before 4.0 there is no flag at all and both bounds are used, which prunes the page away: 25 rows of NaN return 0 for WHERE v > 5.

The bounds round-trip exactly, so neither the flags nor the parse failure #67341 keys on can tell such a zone map from a sound one. Mark it pass_all when the bounds come back reversed, which only FLOAT and DOUBLE can do -- any value of any other type moves both. Do the same in flush() and finish() so newly written pages and segments record that they have no bounds instead of leaving it to the reader.

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

Test
- 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
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

hello-stephen · 2026-09-02T06:22:29Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

What problem was fixed (it's best to include specific error reporting information). How it was fixed.
Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
What features were added. Why was this function added?
Which code was refactored and why was this part of the code refactored?
Which functions were optimized and what is the difference before and after the optimization?

csun5285 · 2026-09-02T06:22:43Z

/review

github-actions

Review status: complete after two convergence rounds on head 625ecd3. I found two distinct issues, both left inline.

Critical checkpoints:

Goal and proof: detecting reversed FLOAT/DOUBLE sentinel bounds is the right mechanism, and the new unit cases cover current writer output plus direct FLOAT/DOUBLE and nullable deserialization. The implementation does not yet safely meet the full compatibility goal because of the two inline cases.
Scope and lifecycle: the change is focused. The per-writer page/reset/segment lifecycle, all finite/non-finite/null page orderings, and the merge-before-page-invalidation ordering were traced and are conservative. There is no shared-state concurrency, lock-order, static-initialization, ownership, or configuration concern.
Parallel consumers: current-head classic predicates, expression zonemap filtering, delete predicates, predicate elimination, and pushed-down MIN/MAX all fence pass_all before consuming invalid bounds. Released 4.1 readers do not have the segment statistics fence, which creates the mixed-version issue noted inline.
Persistence and compatibility: no FE/BE variable, EditLog, transaction, or storage-format schema field is added, but the new segment-level pass_all writer state is an incompatible use of the existing protobuf during rolling upgrades.
Tests and results: the added BE unit tests cover current metadata and several historical reversed encodings, but not the old-reader transition or flag-overridden .16g bounds. Per the review-runner instruction, I did not build or execute tests; the live Clang Formatter check is passing.
Performance and observability: the legacy ignored-bound case can unnecessarily disable pruning for an otherwise sound historical map. No new metric or log is needed for the local validation logic.

User focus: no additional focus was supplied, so the full PR was reviewed.

Verdict: request changes for the rolling-upgrade correctness issue and the historical pruning regression.

github-actions · 2026-09-02T07:03:27Z

+ if (_segment_zone_map.has_not_null &&
+ is_reversed(_segment_zone_map.min_value, _segment_zone_map.max_value,
+ _data_type->get_storage_field_type())) {
+ _segment_zone_map.pass_all = true;


[P1] Keep segment metadata readable by older BEs
This starts writing segment-level pass_all=true, which makes to_proto() clear both bounds. Released 4.1 readers do not have the segment_zone_maps_can_answer_agg() fence added in #67341: they still select VStatisticsIterator for pushed-down MIN/MAX, skip parsing the cleared strings because pass_all is set, and then insert both bounds. For a non-nullable FLOAT/DOUBLE all-NaN (or single-infinity) segment, one side remains a default TYPE_NULL Field, so an older BE reading a segment produced by an upgraded BE can consume uninitialized storage and return a wrong aggregate during rolling upgrade. This is reachable through shared cloud segments and replica snapshot/clone. Please keep the legacy segment encoding until every supported reader fences pass_all, or version-gate this on-disk transition. The reader-side reversal check can still repair existing metadata, and page-level pass_all is already established.

NaN and infinity are recorded in the has_nan/has_positive_inf/ has_negative_inf flags and never move min/max, so a page holding nothing else leaves both at the values add_values() starts from: min = DBL_MAX and max = -DBL_MAX. Those are written out as if they were data, and the flags stand in for at most one side, so reading them back reports a DBL_MAX that is nowhere in the page -- SELECT MIN(v) answers with it while SELECT COUNT(*) WHERE v = that value answers 0. On data written before 4.0 there is no flag at all and both bounds are used, which prunes the page away: 25 rows of NaN return 0 for WHERE v > 5. The bounds round-trip exactly, so neither the flags nor the parse failure apache#67341 keys on can tell such a zone map from a sound one. Comparing against the values the writer starts from does not work either: 4.0 and 4.1 wrote them with digits10 + 1 digits, so a FLOAT page recorded 3.402823e+38, which no longer equals FLT_MAX but is still reversed. Mark the zone map pass_all when its bounds come back reversed, which only FLOAT and DOUBLE can do -- any value of any other type moves both bounds. Reversed is exactly the state of a page that held no finite value, so the test walks all 127 non-empty subsets of {NaN, +inf, -inf, max, lowest, 1.5, 20.5} through the writer and checks that each zone map either reports itself unusable or covers every value the page holds, and that the seven subsets without a finite value are the only unusable ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPTGDEAgu5sQVYYvfvyTJz

csun5285 · 2026-09-04T08:04:53Z

/review

csun5285 · 2026-09-04T08:05:00Z

run buildall

github-actions

Review status: complete after one convergence round on head b39c77f. Both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS, and the main final sweep found no distinct inline issue.

Critical checkpoints:

Goal and proof: the change detects parseable reversed FLOAT/DOUBLE sentinel bounds during ZoneMap::from_proto and conservatively marks the map pass_all. The 127 non-empty special/finite subsets for each floating type, current encodings, legacy truncated FLOAT bounds, flag combinations, sound controls, and nullable/non-nullable deserialization cover the intended mechanism and fail on the base behavior.
Scope and clarity: the production change is focused on one typed comparison helper and the deserialization order; the larger test addition is a bounded exhaustive property check.
Concurrency: no shared mutable state, threads, locks, atomics, or lock ordering are introduced. Deserialization operates on call-local state.
Lifecycle and ownership: no new resource lifetime, static initialization dependency, ownership cycle, or allocation-tracking concern exists. Test writers are closed and fixture teardown removes their files.
Configuration: no configuration item is added or changed. The test only reads the existing zone-map row threshold.
Compatibility: current head changes only reader interpretation and does not persist segment-level pass_all or alter the protobuf/storage format. The writer-side hunk behind discussion r3911576164 is absent, so that new older-reader rolling-upgrade regression is resolved.
Parallel consumers: classic segment/page predicates, delete predicates, expression zone-map filtering, predicate elimination, and pushed-down non-COUNT aggregates all fence pass_all. MIN/MAX falls back before VStatisticsIterator; COUNT does not read bounds; nullness-only evaluation uses valid null flags.
Test coverage and results: the added BE unit cases are deterministic, bounded, and exercise page-to-segment construction plus direct historical deserialization. No result file is involved. Per the review-only runner instruction, I did not build or run tests locally. Live Clang Formatter and repository checks passed. The COMPILE job failed before build.sh because the build container timed out connecting to gitee.com while installing nvm, so it produced no compiler diagnostic; BE UT remained pending at the final audit.
Observability: no new log or metric is needed for this local conservative validation path.
Transactions, persistence, and writes: no transaction, EditLog, visibility, delete-bitmap, or data-write protocol is modified; only existing zone-map metadata is interpreted more safely.
FE/BE propagation: no new cross-process variable, thrift field, or execution-version behavior is introduced.
Performance: repaired maps fall back to data reads as intended. Parsing a flag-overridden legacy bound can also disable an otherwise usable map, but that exact pruning loss is already covered by discussion r3911576155 and the author explicitly accepted invalidation; no distinct correctness or availability consequence was found.
Other risks: no memory-safety, error-propagation, nullability-type, special-value ordering, or parallel-path defect remains after the missed-issue rechecks.

Existing review context: r3911576155 remains the duplicate fence for the acknowledged conservative pruning degradation; r3911576164 is resolved by removal of writer-side segment pass_all and was not repeated.

User focus: no additional focus was supplied, so the full PR was reviewed.

Verdict: approval opinion from the code-review standpoint; no distinct blocking issue remains on the reviewed head.

hello-stephen · 2026-09-04T08:57:14Z

TPC-H: Total hot run time: 16886 ms

machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false
------ Round 1 ----------------------------------
============================================
q1	17615	3024	3027	3024
q2	2082	262	227	227
q3	10244	896	511	511
q4	4672	247	200	200
q5	7680	676	385	385
q6	136	116	94	94
q7	542	538	380	380
q8	9263	957	883	883
q9	3442	2425	2382	2382
q10	6493	847	695	695
q11	397	197	178	178
q12	617	258	209	209
q13	18113	1511	1159	1159
q14	157	146	130	130
q15	q16	429	394	362	362
q17	1384	835	823	823
q18	3086	2261	2261	2261
q19	1268	878	771	771
q20	373	290	197	197
q21	5586	1788	1900	1788
q22	319	269	227	227
Total cold run time: 93898 ms
Total hot run time: 16886 ms
----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3368	3294	3287	3287
q2	503	381	367	367
q3	2232	2255	2196	2196
q4	1179	1155	886	886
q5	2178	2146	2101	2101
q6	168	117	85	85
q7	1013	910	864	864
q8	1573	1396	1386	1386
q9	3112	3101	3102	3101
q10	1855	1784	1635	1635
q11	349	266	245	245
q12	450	434	350	350
q13	1462	1509	1142	1142
q14	172	169	157	157
q15	q16	380	392	364	364
q17	3645	3293	3187	3187
q18	4773	4386	4682	4386
q19	849	815	912	815
q20	1015	958	812	812
q21	3843	3110	3294	3110
q22	387	351	326	326
Total cold run time: 34506 ms
Total hot run time: 30802 ms

hello-stephen · 2026-09-04T09:03:42Z

TPC-DS: Total hot run time: 81774 ms

machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false
query5	4250	408	327	327
query6	384	136	120	120
query7	4969	430	236	236
query8	280	126	117	117
query9	8682	2883	2866	2866
query10	388	218	180	180
query11	5378	1022	892	892
query12	115	69	70	69
query13	1193	450	335	335
query14	6167	2161	2064	2064
query14_1	1958	1953	1939	1939
query15	176	120	110	110
query16	914	367	339	339
query17	791	463	367	367
query18	2332	343	246	246
query19	169	139	111	111
query20	71	71	73	71
query21	198	101	88	88
query22	5449	5346	5364	5346
query23	6785	6141	5928	5928
query23_1	6263	6258	6054	6054
query24	7288	1080	764	764
query24_1	796	778	780	778
query25	447	321	258	258
query26	1242	247	128	128
query27	2779	437	266	266
query28	4656	1513	1491	1491
query29	926	455	362	362
query30	252	158	131	131
query31	812	400	331	331
query32	126	72	79	72
query33	453	217	192	192
query34	997	830	512	512
query35	410	405	351	351
query36	561	607	531	531
query37	119	81	71	71
query38	989	855	796	796
query39	495	509	475	475
query39_1	455	457	453	453
query40	198	92	79	79
query41	59	60	58	58
query42	78	73	72	72
query43	239	237	211	211
query44	1031	554	556	554
query45	115	105	102	102
query46	747	815	562	562
query47	749	753	700	700
query48	302	284	227	227
query49	545	233	183	183
query50	716	260	190	190
query51	8214	8010	8242	8010
query52	66	70	57	57
query53	194	200	144	144
query54	216	199	161	161
query55	70	62	71	62
query56	261	159	168	159
query57	680	660	651	651
query58	202	159	159	159
query59	1196	1237	1092	1092
query60	234	180	167	167
query61	116	111	141	111
query62	347	200	172	172
query63	169	133	140	133
query64	2678	694	574	574
query65	1573	1533	1600	1533
query66	1836	261	206	206
query67	9979	9951	9716	9716
query68	2995	1191	704	704
query69	353	226	203	203
query70	671	619	622	619
query71	250	181	162	162
query72	2347	1705	1574	1574
query73	663	643	343	343
query74	2000	1220	1121	1121
query75	1176	1094	960	960
query76	2389	725	554	554
query77	248	254	203	203
query78	4220	3843	3232	3232
query79	1175	819	547	547
query80	1203	310	286	286
query81	489	156	133	133
query82	625	122	95	95
query83	278	204	196	196
query84	292	115	90	90
query85	807	363	302	302
query86	396	172	172	172
query87	1022	953	885	885
query88	2755	2094	2124	2094
query89	286	197	173	173
query90	1995	126	131	126
query91	139	126	101	101
query92	77	69	69	69
query93	1257	1090	644	644
query94	636	258	209	209
query95	507	265	222	222
query96	815	559	260	260
query97	1050	1051	1056	1051
query98	143	134	132	132
query99	427	337	321	321
Total cold run time: 176637 ms
Total hot run time: 81774 ms

hello-stephen · 2026-09-04T09:08:04Z

ClickBench: Total hot run time: 14.67 s

machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit b39c77f3e7ee1aee21d4a8ad6ce21ae200dfea7b, data reload: false
query1	0.01	0.00	0.00
query2	0.08	0.04	0.04
query3	0.24	0.10	0.10
query4	1.60	0.11	0.11
query5	0.18	0.18	0.16
query6	1.23	0.69	0.67
query7	0.04	0.01	0.00
query8	0.05	0.03	0.03
query9	0.30	0.21	0.22
query10	0.35	0.33	0.34
query11	0.16	0.11	0.12
query12	0.15	0.12	0.12
query13	0.31	0.32	0.31
query14	0.45	0.45	0.45
query15	0.35	0.35	0.35
query16	0.21	0.22	0.24
query17	0.61	0.69	0.73
query18	0.18	0.16	0.17
query19	1.24	1.21	1.12
query20	0.01	0.01	0.01
query21	15.44	0.17	0.13
query22	5.05	0.05	0.04
query23	16.19	0.24	0.11
query24	3.06	0.32	0.26
query25	0.12	0.04	0.03
query26	0.82	0.15	0.12
query27	0.03	0.03	0.02
query28	3.66	0.51	0.29
query29	12.47	3.16	2.58
query30	0.25	0.12	0.11
query31	2.75	0.38	0.17
query32	3.46	0.31	0.22
query33	1.36	1.35	1.62
query34	15.36	2.22	1.81
query35	1.76	1.75	1.73
query36	0.45	0.30	0.29
query37	0.06	0.04	0.04
query38	0.04	0.03	0.02
query39	0.03	0.03	0.02
query40	0.11	0.07	0.07
query41	0.07	0.02	0.02
query42	0.04	0.02	0.03
query43	0.03	0.03	0.03
Total cold run time: 90.36 s
Total hot run time: 14.67 s

hello-stephen · 2026-09-04T13:57:31Z

BE Regression && UT Coverage Report

Increment line coverage 100.00% (17/17) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	76.29% (34655/45427)
Line Coverage	61.36% (391122/637438)
Region Coverage	57.56% (328947/571459)
Branch Coverage	58.40% (149999/256842)

eldenmoon

lgtm

github-actions · 2026-09-04T14:45:11Z

PR approved by anyone and no changes requested.

github-actions · 2026-09-04T14:45:14Z

PR approved by at least one committer and no changes requested.

NaN and infinity are recorded in the has_nan/has_positive_inf/ has_negative_inf flags and never move min/max, so a page holding nothing else leaves both at the values add_values() starts from: min = DBL_MAX and max = -DBL_MAX. Those are written out as if they were data, and the flags stand in for at most one side, so reading them back reports a DBL_MAX that is nowhere in the page -- SELECT MIN(v) answers with it while SELECT COUNT(*) WHERE v = that value answers 0. On data written before 4.0 there is no flag at all and both bounds are used, which prunes the page away: 25 rows of NaN return 0 for WHERE v > 5. The bounds round-trip exactly, so neither the flags nor the parse failure #67341 keys on can tell such a zone map from a sound one. Mark it pass_all when the bounds come back reversed, which only FLOAT and DOUBLE can do -- any value of any other type moves both. Do the same in flush() and finish() so newly written pages and segments record that they have no bounds instead of leaving it to the reader.

csun5285 requested review from airborne12, eldenmoon, gavinchou and yiguolei as code owners September 2, 2026 06:22

github-actionsBot previously requested changes Sep 2, 2026
View reviewed changes

csun5285 mentioned this pull request Sep 2, 2026
branch-4.0:[fix](zonemap) Make DOUBLE/FLOAT zone map bounds usable ( #65302 #67341 #67431) #67440
Open
16 tasks

csun5285 force-pushed the fix-zonemap-reversed-bounds branch from 625ecd3 to b39c77fCompare September 4, 2026 07:44

github-actionsBot reviewed Sep 4, 2026
View reviewed changes

yiguolei approved these changes Sep 4, 2026
View reviewed changes

eldenmoon approved these changes Sep 4, 2026
View reviewed changes

csun5285 merged commit 43db29b into apache:masterSep 4, 2026
34 checks passed

github-actionsBot added the reviewed label Sep 4, 2026

github-actionsBot added the approved Indicates a PR has been approved by one committer. label Sep 4, 2026

yiguolei added the dev/4.1.x label Sep 6, 2026

github-actionsBot mentioned this pull request Sep 6, 2026
branch-4.1: [fix](zonemap) Treat reversed zone map bounds as invalid #67431 #67550
Open

Uh oh!

Conversation

csun5285 commented Sep 2, 2026

What problem does this PR solve?

Release note

Check List (For Author)

Check List (For Reviewer who merge this PR)

Uh oh!

hello-stephen commented Sep 2, 2026

Uh oh!

csun5285 commented Sep 2, 2026

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actionsBotSep 2, 2026

Choose a reason for hiding this comment

Uh oh!

csun5285 commented Sep 4, 2026

Uh oh!

csun5285 commented Sep 4, 2026

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

hello-stephen commented Sep 4, 2026

Uh oh!

hello-stephen commented Sep 4, 2026

Uh oh!

hello-stephen commented Sep 4, 2026

Uh oh!

hello-stephen commented Sep 4, 2026

BE Regression && UT Coverage Report

Uh oh!

eldenmoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actionsBot commented Sep 4, 2026

Uh oh!

github-actionsBot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants