Skip to content

[fix](exec) preserve NULL in pushed-down CHAR MIN/MAX - #65952

Open
Mryange wants to merge 1 commit into
apache:masterfrom
Mryange:fix-nullable-char-minmax
Open

[fix](exec) preserve NULL in pushed-down CHAR MIN/MAX#65952
Mryange wants to merge 1 commit into
apache:masterfrom
Mryange:fix-nullable-char-minmax

Conversation

@Mryange

@Mryange Mryange commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary: MIN on a nullable CHAR column could return an empty string when string min/max aggregation was pushed down to multiple tablets. The zone-map reader produced NULL correctly, but the CHAR padding cleanup rebuilt each value through a non-null Field, converting StringRef{nullptr, 0} into an empty string. Preserve the original StringRef through insert_data() so NULL remains NULL while CHAR padding is still removed.

Release note

Fix incorrect MIN results for nullable CHAR columns when min/max aggregation pushdown is enabled.

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

Copy link
Copy Markdown
Contributor

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:

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

@Mryange

Mryange commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 28th, 2026 5:08 PM.
Workflow run: https://github.com/apache/doris/actions/runs/29989313820

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

HappenLee
HappenLee previously approved these changes Jul 26, 2026

@HappenLee HappenLee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code is ok, remove the pr.md

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Jul 26, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@Mryange

Mryange commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Jul 26, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review opinion: approval; no blocking issues or inline findings.

Critical checkpoint conclusions:

  • Goal and test proof: the change fixes the stated nullable CHAR MIN/MAX pushdown bug at the representation boundary. An all-NULL zone map reaches VStatisticsIterator as StringRef {nullptr, 0}; inserting that reference directly into the cloned ColumnNullable preserves the null map instead of rebuilding a non-NULL empty Field. The new BE unit test writes a real all-NULL nullable CHAR segment, exercises the MINMAX path, verifies both statistics rows remain NULL, and verifies EOF. Its null assertions would fail with the old conversion.
  • Scope and code quality: the production change is a focused two-line replacement and the test is isolated to the affected iterator. All newly used Status results are checked. The implementation continues to trim only trailing CHAR padding and also preserves genuine non-NULL empty strings because canonical ColumnStr storage supplies a non-NULL data pointer.
  • Concurrency and lifecycle: this change adds no shared mutable state, locks, atomics, or static initialization. The source column remains owned and unchanged while the independent clone is populated, then the clone is swapped into the block.
  • Compatibility and parallel paths: there are no protocol, symbol, storage-format, configuration, persistence, transaction, delete-bitmap, or rolling-upgrade changes. File-backed, missing/default, and current constant column iterators all populate the same schema-created destination before the common CHAR normalization. No parallel statistics implementation requires a matching fix.
  • End-to-end correctness: the production Segment -> VStatisticsIterator -> rowset/tablet direct path forwards the corrected nullable extrema to the unchanged upper MIN/MAX aggregate, so the focused one-segment test proves the exact contract behind the reported multi-tablet symptom.
  • Performance and observability: direct insertion removes temporary std::string and Field construction for the two CHAR statistics rows per segment. No new logging, metrics, or documentation are warranted for this local representation correction.
  • Validation status: the authoritative patch and related call paths were reviewed statically as required by the automation contract; no local build or test was attempted. The live Clang Formatter check is passing, while BE UT (macOS) is still pending at review time.
  • User focus: no additional review focus was provided.

The full normal and risk-focused review passes converged with no new valuable findings.

@Mryange

Mryange commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 29823 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit ad868e6371d935ceb4c8dede28ea547861e7c252, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17926	4239	4428	4239
q2	2038	333	212	212
q3	10252	1469	865	865
q4	4689	469	338	338
q5	7514	859	588	588
q6	185	189	143	143
q7	775	821	620	620
q8	9670	1622	1502	1502
q9	5714	4381	4340	4340
q10	6786	1775	1469	1469
q11	519	355	322	322
q12	758	577	445	445
q13	18102	3447	2814	2814
q14	266	262	252	252
q15	q16	787	787	707	707
q17	1009	922	1042	922
q18	6898	5747	5635	5635
q19	1172	1248	1090	1090
q20	787	694	615	615
q21	5514	2663	2403	2403
q22	425	354	302	302
Total cold run time: 101786 ms
Total hot run time: 29823 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4391	4332	4315	4315
q2	286	320	205	205
q3	4588	4952	4397	4397
q4	2067	2126	1366	1366
q5	4408	4261	4332	4261
q6	234	178	126	126
q7	1742	2231	1752	1752
q8	2637	2187	2252	2187
q9	8027	7999	7777	7777
q10	4696	4688	4244	4244
q11	560	402	386	386
q12	742	776	530	530
q13	3245	3630	2959	2959
q14	279	301	279	279
q15	q16	708	734	650	650
q17	1387	1315	1447	1315
q18	7817	7304	7249	7249
q19	1174	1122	1117	1117
q20	2236	2216	1958	1958
q21	5239	4580	4456	4456
q22	544	452	400	400
Total cold run time: 57007 ms
Total hot run time: 51929 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 177076 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 ad868e6371d935ceb4c8dede28ea547861e7c252, data reload: false

query5	4319	617	478	478
query6	467	222	208	208
query7	4833	598	366	366
query8	342	194	173	173
query9	8768	4022	4008	4008
query10	476	343	301	301
query11	5986	2297	2126	2126
query12	159	103	101	101
query13	1275	612	419	419
query14	6236	5150	4894	4894
query14_1	4239	4245	4240	4240
query15	218	209	183	183
query16	1052	490	443	443
query17	1129	732	574	574
query18	2462	451	334	334
query19	196	180	138	138
query20	112	107	112	107
query21	238	156	131	131
query22	13581	13447	13409	13409
query23	17344	16488	16089	16089
query23_1	16305	16141	16241	16141
query24	7482	1743	1274	1274
query24_1	1314	1286	1280	1280
query25	556	451	357	357
query26	1321	353	214	214
query27	2586	595	373	373
query28	4466	1972	1978	1972
query29	1050	611	482	482
query30	343	266	223	223
query31	1111	1088	978	978
query32	113	64	57	57
query33	505	318	247	247
query34	1167	1138	662	662
query35	775	780	674	674
query36	1215	1229	1040	1040
query37	151	105	92	92
query38	1874	1703	1668	1668
query39	883	872	894	872
query39_1	862	844	837	837
query40	253	164	141	141
query41	64	63	62	62
query42	93	89	89	89
query43	324	322	282	282
query44	1412	775	753	753
query45	206	184	170	170
query46	1125	1166	719	719
query47	2208	2153	2059	2059
query48	376	411	300	300
query49	582	418	304	304
query50	1073	429	346	346
query51	10733	10689	10435	10435
query52	84	86	74	74
query53	259	274	197	197
query54	277	228	214	214
query55	73	70	64	64
query56	309	299	304	299
query57	1307	1337	1219	1219
query58	282	259	250	250
query59	1582	1658	1463	1463
query60	315	269	251	251
query61	147	154	149	149
query62	554	499	432	432
query63	235	200	203	200
query64	2835	1084	833	833
query65	4659	4650	4637	4637
query66	1845	502	382	382
query67	29423	29311	29073	29073
query68	2990	1548	952	952
query69	400	300	261	261
query70	1035	935	907	907
query71	374	339	342	339
query72	3050	2733	2445	2445
query73	813	767	420	420
query74	5029	4935	4724	4724
query75	2522	2524	2132	2132
query76	2326	1189	802	802
query77	338	374	281	281
query78	11840	11999	11225	11225
query79	1217	1164	750	750
query80	596	542	451	451
query81	452	327	302	302
query82	237	155	124	124
query83	316	319	294	294
query84	270	164	127	127
query85	898	606	522	522
query86	311	294	282	282
query87	1819	1812	1740	1740
query88	3652	2771	2750	2750
query89	418	379	331	331
query90	2179	203	184	184
query91	207	194	154	154
query92	62	58	57	57
query93	1481	1558	971	971
query94	522	353	328	328
query95	784	591	476	476
query96	1051	773	333	333
query97	2611	2594	2504	2504
query98	219	204	206	204
query99	1077	1104	966	966
Total cold run time: 261559 ms
Total hot run time: 177076 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.94 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit ad868e6371d935ceb4c8dede28ea547861e7c252, data reload: false

query1	0.01	0.00	0.01
query2	0.10	0.05	0.05
query3	0.25	0.14	0.14
query4	1.60	0.14	0.13
query5	0.24	0.22	0.22
query6	1.27	1.04	1.03
query7	0.04	0.01	0.01
query8	0.06	0.04	0.04
query9	0.37	0.30	0.31
query10	0.54	0.55	0.54
query11	0.19	0.14	0.13
query12	0.17	0.15	0.14
query13	0.47	0.48	0.47
query14	1.01	1.02	1.02
query15	0.60	0.59	0.59
query16	0.32	0.30	0.30
query17	1.06	1.04	1.10
query18	0.23	0.22	0.21
query19	1.98	1.92	2.01
query20	0.02	0.01	0.02
query21	15.42	0.19	0.13
query22	4.96	0.06	0.05
query23	16.17	0.30	0.12
query24	3.03	0.45	0.32
query25	0.11	0.05	0.04
query26	0.72	0.21	0.14
query27	0.04	0.03	0.03
query28	3.58	0.86	0.52
query29	12.49	4.09	3.28
query30	0.27	0.15	0.16
query31	2.77	0.58	0.31
query32	3.23	0.60	0.49
query33	3.13	3.22	3.20
query34	15.71	4.28	3.56
query35	3.52	3.52	3.54
query36	0.56	0.43	0.43
query37	0.09	0.07	0.06
query38	0.04	0.04	0.04
query39	0.04	0.03	0.03
query40	0.18	0.17	0.15
query41	0.09	0.03	0.02
query42	0.04	0.03	0.03
query43	0.04	0.03	0.03
Total cold run time: 96.76 s
Total hot run time: 24.94 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 100.00% (2/2) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 58.14% (24639/42379)
Line Coverage 42.22% (246325/583462)
Region Coverage 38.10% (195616/513387)
Branch Coverage 39.19% (88185/225008)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (2/2) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 75.35% (31139/41325)
Line Coverage 59.92% (347304/579604)
Region Coverage 56.71% (292161/515186)
Branch Coverage 58.04% (130608/225043)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants