Skip to content

[refactor](pb) remove default value from column meta pb - #68323

Open
yiguolei wants to merge 3 commits into
apache:masterfrom
yiguolei:f3
Open

yiguolei wants to merge 3 commits into
apache:masterfrom
yiguolei:f3

Conversation

@yiguolei

Copy link
Copy Markdown
Contributor

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

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?

@yiguolei

Copy link
Copy Markdown
Contributor Author

run buildall

@yiguolei

Copy link
Copy Markdown
Contributor Author

/review

csun5285
csun5285 previously approved these changes Sep 21, 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.

Review conclusion: request changes for one persisted-format compatibility gap.

Critical checkpoints:

  • Goal and correctness: both current ColumnMetaPB.default_value producers are removed, no current reader consumes the field, and schema-evolution defaults still come from TabletColumn; the runtime removal itself is sound.
  • Scope and parallel paths: both top-level/vertical and variant subcolumn producers are covered. Static reconstruction of all 148 changed segment fixtures found 146 changed only by recursive tag-14 removal plus derived protobuf lengths/checksum; the other two only reorder unchanged partial-update page blocks along the test's documented normalization path.
  • Persistence and rolling compatibility: old-writer/new-reader and new-writer/old-reader are currently readable, but tag 14 is not formally reserved and every old tag-bearing fixture was regenerated. The inline comment requests an enforced reservation and explicit V2-inline/V3-external legacy-read coverage.
  • Concurrency, lifecycle, configuration, transactions, memory, errors, observability, and performance: this patch adds no concurrent state, locks, configuration, transaction flow, allocation ownership, or new error/observability path. Its intended performance effect is smaller segment metadata.
  • Tests: the new-writer golden corpus spans 73 cases/148 segments, including V2/V3, horizontal/vertical, variant, row store, indexes, partial update, binlog, MoW/MoR, cluster and sequence-key paths. It no longer exercises legacy wire tag 14. No local build or test was run because the review environment forbids it; compile and BE/FE/Cloud CI were still pending at submission time.
  • User focus: no additional focus was provided; the complete PR was reviewed.

Two full convergence rounds completed. Round 2's normal runtime, normal tests, and separate risk-focused scans all returned NO_NEW_VALUABLE_FINDINGS beyond the single merged comment.


// Extra type info to be compatible with tabet schema
optional bytes default_value = 14; // ColumnMessage.default_value ?
// optional bytes default_value = 14; // ColumnMessage.default_value, this field is useless, it is added by https://github.com/apache/doris/pull/24170/

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.

ColumnMetaPB is persisted in both V2 footers and V3 external metadata, and the pre-change corpus contains tag 14 2,437 times. Turning the declaration into a comment removes it from the descriptor, so a future field can legally reuse 14 and reinterpret old bytes. Please add reserved 14; (and reserve the old name "default_value" as schema hygiene).

Please also add explicit old-writer/new-reader coverage for both V2-inline and V3-external metadata, either with representative pre-change segments or targeted raw-tag injection. Regenerating all 148 goldens removes the only fixtures containing this tag, including non-empty and binary defaults. The runtime removal itself is otherwise compatible because readers source schema-evolution defaults from TabletColumn.

@hello-stephen

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

------ Round 1 ----------------------------------
============================================
q1	16983	3873	3855	3855
q2	2190	365	295	295
q3	9962	1407	781	781
q4	4672	480	349	349
q5	7565	876	555	555
q6	186	172	134	134
q7	789	820	600	600
q8	9417	1568	1483	1483
q9	5477	4218	4214	4214
q10	6825	1327	1029	1029
q11	442	279	244	244
q12	628	421	295	295
q13	18045	2618	1985	1985
q14	258	263	232	232
q15	q16	739	713	673	673
q17	1810	1094	1042	1042
q18	6505	5617	5530	5530
q19	1183	1314	1094	1094
q20	481	381	258	258
q21	5493	3136	3105	3105
q22	443	371	310	310
Total cold run time: 100093 ms
Total hot run time: 28063 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4605	4648	4438	4438
q2	729	576	550	550
q3	4782	5214	4496	4496
q4	2249	2325	1434	1434
q5	4458	4403	4494	4403
q6	228	180	124	124
q7	1840	1669	1508	1508
q8	2297	2152	1996	1996
q9	7372	7218	7193	7193
q10	3662	3591	3166	3166
q11	535	381	341	341
q12	703	700	495	495
q13	2300	2598	1968	1968
q14	256	266	260	260
q15	q16	656	675	589	589
q17	7321	6735	6631	6631
q18	11865	11087	11745	11087
q19	1077	988	1035	988
q20	2215	2173	1907	1907
q21	4980	4077	4273	4077
q22	496	434	392	392
Total cold run time: 64626 ms
Total hot run time: 58043 ms

@hello-stephen

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

query5	4324	599	448	448
query6	435	215	188	188
query7	4800	517	303	303
query8	321	177	176	176
query9	8791	3974	3947	3947
query10	456	312	262	262
query11	5896	3543	3234	3234
query12	150	92	89	89
query13	1273	647	419	419
query14	6528	4454	4189	4189
query14_1	3982	3966	3948	3948
query15	206	197	177	177
query16	1026	503	461	461
query17	928	689	551	551
query18	2437	470	340	340
query19	206	184	143	143
query20	105	86	81	81
query21	223	134	117	117
query22	12952	12941	12800	12800
query23	14001	13005	12529	12529
query23_1	12469	12586	12543	12543
query24	7101	1143	687	687
query24_1	646	699	675	675
query25	562	433	386	386
query26	1271	306	171	171
query27	2690	560	361	361
query28	4490	1999	1991	1991
query29	1612	742	533	533
query30	291	207	187	187
query31	904	758	636	636
query32	151	99	97	97
query33	543	310	262	262
query34	1216	1080	647	647
query35	726	747	660	660
query36	814	809	717	717
query37	145	104	93	93
query38	1814	1769	1679	1679
query39	706	677	651	651
query39_1	635	664	664	664
query40	240	129	105	105
query41	73	73	69	69
query42	104	93	94	93
query43	331	347	299	299
query44	1382	722	724	722
query45	184	179	166	166
query46	1113	1210	708	708
query47	1509	1503	1400	1400
query48	447	392	285	285
query49	572	401	290	290
query50	977	361	257	257
query51	10732	10633	10422	10422
query52	87	86	74	74
query53	244	254	181	181
query54	243	207	180	180
query55	78	74	73	73
query56	220	205	229	205
query57	1445	1416	1353	1353
query58	282	264	256	256
query59	1984	2064	1860	1860
query60	272	244	217	217
query61	140	150	150	150
query62	389	317	264	264
query63	216	171	171	171
query64	2808	998	801	801
query65	3490	3402	3392	3392
query66	1785	409	296	296
query67	20130	20016	21656	20016
query68	3627	1419	945	945
query69	424	306	260	260
query70	909	819	808	808
query71	301	231	213	213
query72	2819	2493	2257	2257
query73	822	776	419	419
query74	4636	4454	4300	4300
query75	2314	2279	1923	1923
query76	2254	1087	715	715
query77	344	395	288	288
query78	9095	9171	8515	8515
query79	1372	1090	755	755
query80	605	454	361	361
query81	546	334	284	284
query82	623	157	122	122
query83	283	222	196	196
query84	321	143	115	115
query85	950	472	388	388
query86	326	244	227	227
query87	1999	1971	1833	1833
query88	3621	2724	2718	2718
query89	388	287	240	240
query90	1819	179	183	179
query91	173	156	134	134
query92	107	90	89	89
query93	1572	1568	901	901
query94	522	333	312	312
query95	663	381	329	329
query96	1037	789	320	320
query97	2414	2408	2338	2338
query98	158	149	145	145
query99	707	722	606	606
Total cold run time: 236647 ms
Total hot run time: 152870 ms

@hello-stephen

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

query1	0.00	0.00	0.00
query2	0.10	0.05	0.05
query3	0.26	0.13	0.14
query4	1.61	0.14	0.14
query5	0.25	0.22	0.21
query6	1.16	0.92	0.93
query7	0.04	0.01	0.00
query8	0.05	0.04	0.03
query9	0.40	0.32	0.34
query10	0.59	0.56	0.58
query11	0.23	0.15	0.14
query12	0.19	0.15	0.15
query13	0.46	0.46	0.46
query14	0.96	0.92	0.95
query15	0.59	0.57	0.59
query16	0.31	0.32	0.33
query17	1.11	1.08	1.08
query18	0.21	0.20	0.19
query19	2.06	1.96	1.94
query20	0.02	0.01	0.01
query21	15.48	0.17	0.14
query22	5.01	0.04	0.05
query23	16.15	0.30	0.12
query24	3.05	0.44	0.29
query25	0.11	0.05	0.03
query26	0.74	0.20	0.14
query27	0.04	0.06	0.04
query28	3.48	0.78	0.34
query29	12.48	4.00	3.18
query30	0.28	0.15	0.15
query31	2.77	0.57	0.32
query32	3.23	0.59	0.48
query33	3.13	3.17	3.21
query34	15.64	3.96	3.29
query35	3.22	3.20	3.24
query36	0.55	0.44	0.43
query37	0.10	0.07	0.06
query38	0.05	0.04	0.03
query39	0.04	0.03	0.03
query40	0.18	0.15	0.15
query41	0.08	0.04	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.04
Total cold run time: 96.49 s
Total hot run time: 23.87 s

@yiguolei

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 77.54% (2064/2662)
Line Coverage 65.75% (37931/57686)
Region Coverage 53.18% (35533/66816)
Branch Coverage 56.55% (11418/20190)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants