[fix](storage) Fix linker error for SegmentWriter::_is_mow() with -O1/-O3#63928
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
690328a to
804c31c
Compare
|
PR approved by anyone and no changes requested. |
|
run buildall |
TPC-H: Total hot run time: 29263 ms |
TPC-DS: Total hot run time: 170825 ms |
804c31c to
04717d8
Compare
|
run buildall |
TPC-H: Total hot run time: 28727 ms |
TPC-DS: Total hot run time: 170738 ms |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 29389 ms |
TPC-DS: Total hot run time: 169461 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1c35ec3 to
96f1ab1
Compare
|
run buildall |
TPC-H: Total hot run time: 28971 ms |
TPC-DS: Total hot run time: 169143 ms |
ce47654 to
3369788
Compare
|
run buildall |
3369788 to
77cacdb
Compare
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29465 ms |
TPC-DS: Total hot run time: 169275 ms |
|
PR approved by at least one committer and no changes requested. |
|
/review |
There was a problem hiding this comment.
Code review completed. I did not find blocking issues in this PR.
Critical checkpoint conclusions:
- Goal and tests: The PR addresses the O1/O3 linker failure by moving the helper definitions into the headers where cross-translation-unit callers can see inline definitions. The new BE unit test is included by the existing recursive BE test glob and covers MOW true/false and cluster-key cases for both segment writer classes.
- Scope: The implementation change is small and focused; it only relocates helper definitions and adds a targeted linker-regression test.
- Concurrency: No new concurrency or shared mutable state is introduced.
- Lifecycle/static initialization: No new static initialization dependency was introduced. The test directory constant is local to the test translation unit and does not affect production lifecycle.
- Configuration/compatibility: No new configuration, storage format, RPC, or FE-BE protocol compatibility impact.
- Parallel code paths: Both SegmentWriter and VerticalSegmentWriter are updated consistently.
- Error handling/status: Production error handling is unchanged; the test checks filesystem setup statuses before use.
- Test coverage: Coverage is appropriate for this linker/inline-definition regression. I did not run the BE unit test locally because no BE test binary/build output is available in this runner.
- Observability: Not applicable for this compile/link-only fix.
- Transaction/data correctness: Not applicable; no data visibility, version, rowset lifecycle, or delete bitmap behavior changes.
- Performance/memory: No meaningful runtime performance or memory-accounting risk introduced; helpers are simple predicates that were already intended to be inline-capable.
User focus points: No additional user-provided review focus was specified.
What problem does this PR solve?
Issue Number: close #63927
Problem Summary: Building BE unit tests with TSAN (
-O1) or RELEASE (-O3) causes undefined reference linker errors forSegmentWriter::_is_mow(),SegmentWriter::_is_mow_with_cluster_key(), and theirVerticalSegmentWritercounterparts.Root cause: Commit
74aa0ca63adefined these methods with theinlinekeyword in.cppfiles (segment_writer.cppandvertical_segment_writer.cpp). At-O0(ASAN), the compiler ignoresinlineand exports the symbol, so linking succeeds. At-O1/-O3, the compiler inlines the function and does NOT export the symbol. When commit4616202fc4introducedTestSegmentWriter(a friend subclass) intest_segment_writer.hthat calls these private methods from a different translation unit, the linker cannot find the symbols.Fix: Move the
inlinedefinitions from.cppfiles into the header files as class-internal inline definitions, so they are visible to all translation units.Error example (TSAN build):
Release note
None
Check List (For Author)
Test
segment_writer_mow_check_test.cppwith 7 test cases covering all logic branches for bothSegmentWriterandVerticalSegmentWriterBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)