Skip to content

[fix](test) Fix ODR violation for MockFileReader across test files.#61170

Merged
hello-stephen merged 1 commit intoapache:masterfrom
kaka11chen:fix_OrcMergeRangeFileReaderTest
Mar 10, 2026
Merged

[fix](test) Fix ODR violation for MockFileReader across test files.#61170
hello-stephen merged 1 commit intoapache:masterfrom
kaka11chen:fix_OrcMergeRangeFileReaderTest

Conversation

@kaka11chen
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

Release note

Multiple test files defined different MockFileReader classes with the same fully-qualified name doris::MockFileReader but different member layouts and sizes (96 bytes vs 64 bytes). When linked into the single monolithic doris_be_test binary, the linker would deduplicate symbols and arbitrarily pick one version's template instantiations (e.g., make_shared, destructor), causing size mismatches at runtime. This resulted in ASAN errors:

  • heap-buffer-overflow: allocating 80 bytes but writing 96 bytes
  • new-delete-type-mismatch: allocated 96 bytes, deallocated 64 bytes The ODR conflict was exposed after [be](namespace) remove vecotorized and pipeline namespace #61142 removed the vectorized namespace, which moved orc_file_reader_test.cpp's MockFileReader into namespace doris, colliding with file_meta_cache_test.cpp's MockFileReader in the same namespace.
    Fix: Wrap each MockFileReader class in an anonymous namespace to give them internal linkage, ensuring each translation unit uses its own independent symbols.

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
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?

@kaka11chen
Copy link
Contributor Author

run buildall

yiguolei
yiguolei previously approved these changes Mar 10, 2026
@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Mar 10, 2026
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

Multiple test files defined different MockFileReader classes with the
same fully-qualified name doris::MockFileReader but different member
layouts and sizes (96 bytes vs 64 bytes). When linked into the single
monolithic doris_be_test binary, the linker would deduplicate symbols
and arbitrarily pick one version's template instantiations (e.g.,
make_shared, destructor), causing size mismatches at runtime.
This resulted in ASAN errors:
- heap-buffer-overflow: allocating 80 bytes but writing 96 bytes
- new-delete-type-mismatch: allocated 96 bytes, deallocated 64 bytes
The ODR conflict was exposed after apache#61142 removed the vectorized
namespace, which moved orc_file_reader_test.cpp's MockFileReader
into namespace doris, colliding with file_meta_cache_test.cpp's
MockFileReader in the same namespace.
Fix: Wrap each MockFileReader class in an anonymous namespace to give
them internal linkage, ensuring each translation unit uses its own
independent symbols.
@kaka11chen
Copy link
Contributor Author

run buildall

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Mar 10, 2026
@zclllyybb
Copy link
Contributor

/review

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Mar 10, 2026
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

Code Review Summary

Verdict: No issues found. The change is correct, minimal, and well-targeted.

Critical Checkpoint Conclusions

  1. Goal / Correctness: The PR fixes ODR (One Definition Rule) violations for MockFileReader across 4 test files by wrapping each definition in an anonymous namespace, giving them internal linkage. This is the standard and correct C++ solution. The ASAN heap-buffer-overflow and new-delete-type-mismatch errors described in the PR should be resolved.

  2. Minimal and focused: Yes. Core change is adding namespace { ... } around each MockFileReader class (4 files, +8/-10 lines). The removed code (set_read_status/_read_status in packed_file_reader_test.cpp, last_reader()/_last_reader in packed_file_system_test.cpp) is confirmed dead code with no remaining usages.

  3. Completeness: All 4 MockFileReader definitions in the test codebase are covered by this fix.

  4. Concurrency: Not applicable (test-only mock classes).

  5. Lifecycle / static init: Not applicable. Anonymous namespaces correctly provide internal linkage.

  6. Parallel code paths: Verified all instances are fixed.

  7. Test coverage: The fix is for test infrastructure itself. Existing tests continue to function correctly.

Minor Observation (not blocking)

MockFileWriterForMerge is defined in both packed_file_writer_test.cpp and packed_file_system_test.cpp with identical member layouts but different method implementations (e.g., close() returns _close_status vs Status::OK(), appendv() checks _append_status vs not). This is a similar ODR violation pattern. While member layout matching prevents the same ASAN symptoms, the differing vtable implementations are technically still an ODR violation. Consider wrapping these in anonymous namespaces as a follow-up.

@doris-robot
Copy link

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

------ Round 1 ----------------------------------
============================================
q1	17620	4514	4294	4294
q2	q3	10644	827	506	506
q4	4690	346	248	248
q5	7553	1187	1017	1017
q6	180	178	147	147
q7	775	868	654	654
q8	9296	1467	1327	1327
q9	4779	4736	4763	4736
q10	6256	1923	1620	1620
q11	452	252	243	243
q12	697	564	464	464
q13	18021	2962	2216	2216
q14	233	234	220	220
q15	905	799	812	799
q16	732	727	677	677
q17	723	869	396	396
q18	5940	5367	5267	5267
q19	1251	966	605	605
q20	498	491	386	386
q21	4631	2053	1594	1594
q22	387	322	281	281
Total cold run time: 96263 ms
Total hot run time: 27697 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4726	4589	4465	4465
q2	q3	3860	4364	3819	3819
q4	874	1204	793	793
q5	4046	4377	4342	4342
q6	174	181	142	142
q7	1806	1586	1511	1511
q8	2457	2769	2609	2609
q9	7436	7264	7548	7264
q10	3803	4032	3643	3643
q11	524	432	416	416
q12	517	612	452	452
q13	2712	3287	2334	2334
q14	279	297	266	266
q15	884	811	848	811
q16	829	761	703	703
q17	1152	1403	1386	1386
q18	7057	6808	6506	6506
q19	888	931	878	878
q20	2070	2151	1977	1977
q21	3871	3392	3523	3392
q22	505	422	387	387
Total cold run time: 50470 ms
Total hot run time: 48096 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 153610 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 ba4fb8b496d00a566d54246c2b40ce89a478eff4, data reload: false

query5	4331	671	530	530
query6	329	228	208	208
query7	4239	489	272	272
query8	361	255	244	244
query9	8738	2757	2768	2757
query10	483	385	346	346
query11	7323	5872	5632	5632
query12	186	127	122	122
query13	1281	466	367	367
query14	6285	3853	3583	3583
query14_1	2807	2806	2771	2771
query15	213	202	176	176
query16	1006	453	471	453
query17	1117	742	621	621
query18	2722	451	355	355
query19	222	207	185	185
query20	149	131	133	131
query21	227	157	126	126
query22	4814	4833	4854	4833
query23	16740	15908	15757	15757
query23_1	16003	15849	15778	15778
query24	7379	1685	1350	1350
query24_1	1309	1314	1353	1314
query25	693	496	450	450
query26	1441	290	157	157
query27	3000	507	298	298
query28	4619	1948	1915	1915
query29	886	599	489	489
query30	325	279	240	240
query31	1459	1421	1306	1306
query32	87	74	77	74
query33	570	336	281	281
query34	967	990	587	587
query35	662	703	681	681
query36	1201	1245	1003	1003
query37	137	94	82	82
query38	2949	2955	2937	2937
query39	997	874	847	847
query39_1	819	832	818	818
query40	231	158	136	136
query41	62	61	58	58
query42	304	308	305	305
query43	250	257	225	225
query44	
query45	195	191	181	181
query46	907	999	604	604
query47	2140	2149	2027	2027
query48	299	324	233	233
query49	636	459	380	380
query50	707	275	218	218
query51	4088	4118	4023	4023
query52	289	288	276	276
query53	287	338	285	285
query54	312	270	254	254
query55	89	85	84	84
query56	321	324	309	309
query57	1332	1342	1255	1255
query58	289	280	269	269
query59	1332	1499	1246	1246
query60	331	332	318	318
query61	146	147	147	147
query62	625	595	534	534
query63	303	279	280	279
query64	4979	1288	1000	1000
query65	
query66	1396	472	382	382
query67	16386	16434	16499	16434
query68	
query69	391	301	286	286
query70	997	966	969	966
query71	331	307	298	298
query72	2851	2725	2471	2471
query73	561	564	319	319
query74	9989	9984	9761	9761
query75	2839	2728	2460	2460
query76	2292	1039	663	663
query77	367	382	311	311
query78	11182	11422	10604	10604
query79	1131	808	602	602
query80	1564	616	566	566
query81	567	274	249	249
query82	1193	153	120	120
query83	372	261	243	243
query84	260	118	100	100
query85	1163	474	433	433
query86	429	313	303	303
query87	3172	3124	2958	2958
query88	3568	2667	2664	2664
query89	425	379	344	344
query90	1932	176	172	172
query91	171	164	132	132
query92	74	77	71	71
query93	975	841	510	510
query94	646	300	300	300
query95	591	335	312	312
query96	650	528	232	232
query97	2490	2528	2383	2383
query98	236	225	220	220
query99	1008	1015	916	916
Total cold run time: 236387 ms
Total hot run time: 153610 ms

@hello-stephen
Copy link
Contributor

skip buildall

@hello-stephen hello-stephen merged commit 7bc3230 into apache:master Mar 10, 2026
30 of 32 checks passed
Copy link
Member

@mrhhsg mrhhsg left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants