This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new ad06c797216 [fix](storage) Fix linker error for
SegmentWriter::_is_mow() with -O1/-O3 (#63928)
ad06c797216 is described below
commit ad06c7972164b2bcb34b2732b77025bdd3ed3877
Author: heguanhui <[email protected]>
AuthorDate: Tue Jun 9 09:54:35 2026 +0800
[fix](storage) Fix linker error for SegmentWriter::_is_mow() with -O1/-O3
(#63928)
### 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 for
`SegmentWriter::_is_mow()`, `SegmentWriter::_is_mow_with_cluster_key()`,
and their `VerticalSegmentWriter` counterparts.
**Root cause**: Commit `74aa0ca63a` defined these methods with the
`inline` keyword in `.cpp` files (`segment_writer.cpp` and
`vertical_segment_writer.cpp`). At `-O0` (ASAN), the compiler ignores
`inline` and exports the symbol, so linking succeeds. At `-O1`/`-O3`,
the compiler inlines the function and does NOT export the symbol. When
commit `4616202fc4` introduced `TestSegmentWriter` (a friend subclass)
in `test_segment_writer.h` that calls these private methods from a
different translation unit, the linker cannot find the symbols.
**Fix**: Move the `inline` definitions from `.cpp` files into the header
files as class-internal inline definitions, so they are visible to all
translation units.
**Error example** (TSAN build):
```
undefined reference to 'doris::segment_v2::SegmentWriter::_is_mow()'
undefined reference to
'doris::segment_v2::SegmentWriter::_is_mow_with_cluster_key()'
undefined reference to 'doris::segment_v2::VerticalSegmentWriter::_is_mow()'
undefined reference to
'doris::segment_v2::VerticalSegmentWriter::_is_mow_with_cluster_key()'
```
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test: Added `segment_writer_mow_check_test.cpp` with 7 test
cases covering all logic branches for both `SegmentWriter` and
`VerticalSegmentWriter`
- [ ] Regression 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 <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
Co-authored-by: root <root@DESKTOP-3AF37B>
---
be/src/storage/segment/segment_writer.cpp | 8 -
be/src/storage/segment/segment_writer.h | 8 +-
be/src/storage/segment/vertical_segment_writer.cpp | 8 -
be/src/storage/segment/vertical_segment_writer.h | 8 +-
.../segment/segment_writer_mow_check_test.cpp | 200 +++++++++++++++++++++
5 files changed, 212 insertions(+), 20 deletions(-)
diff --git a/be/src/storage/segment/segment_writer.cpp
b/be/src/storage/segment/segment_writer.cpp
index 00db2362b63..f5f63d923f7 100644
--- a/be/src/storage/segment/segment_writer.cpp
+++ b/be/src/storage/segment/segment_writer.cpp
@@ -1244,13 +1244,5 @@ Status
SegmentWriter::_generate_short_key_index(std::vector<IOlapColumnDataAcces
return Status::OK();
}
-bool SegmentWriter::_is_mow() {
- return _tablet_schema->keys_type() == UNIQUE_KEYS &&
_opts.enable_unique_key_merge_on_write;
-}
-
-bool SegmentWriter::_is_mow_with_cluster_key() {
- return _is_mow() && !_tablet_schema->cluster_key_uids().empty();
-}
-
} // namespace segment_v2
} // namespace doris
diff --git a/be/src/storage/segment/segment_writer.h
b/be/src/storage/segment/segment_writer.h
index c98a6cead20..e6bba442bd4 100644
--- a/be/src/storage/segment/segment_writer.h
+++ b/be/src/storage/segment/segment_writer.h
@@ -189,8 +189,12 @@ private:
IOlapColumnDataAccessor* seq_column, size_t num_rows, bool
need_sort);
Status _generate_short_key_index(std::vector<IOlapColumnDataAccessor*>&
key_columns,
size_t num_rows, const
std::vector<size_t>& short_key_pos);
- bool _is_mow();
- bool _is_mow_with_cluster_key();
+ bool _is_mow() {
+ return _tablet_schema->keys_type() == UNIQUE_KEYS &&
_opts.enable_unique_key_merge_on_write;
+ }
+ bool _is_mow_with_cluster_key() {
+ return _is_mow() && !_tablet_schema->cluster_key_uids().empty();
+ }
protected:
// Build key index for derived writers that override append_block.
diff --git a/be/src/storage/segment/vertical_segment_writer.cpp
b/be/src/storage/segment/vertical_segment_writer.cpp
index fed06831fbd..6cf4cf1a1c6 100644
--- a/be/src/storage/segment/vertical_segment_writer.cpp
+++ b/be/src/storage/segment/vertical_segment_writer.cpp
@@ -1516,12 +1516,4 @@ void VerticalSegmentWriter::_set_max_key(const Slice&
key) {
_max_key.append(key.get_data(), key.get_size());
}
-inline bool VerticalSegmentWriter::_is_mow() {
- return _tablet_schema->keys_type() == UNIQUE_KEYS &&
_opts.enable_unique_key_merge_on_write;
-}
-
-inline bool VerticalSegmentWriter::_is_mow_with_cluster_key() {
- return _is_mow() && !_tablet_schema->cluster_key_uids().empty();
-}
-
} // namespace doris::segment_v2
diff --git a/be/src/storage/segment/vertical_segment_writer.h
b/be/src/storage/segment/vertical_segment_writer.h
index e6ee4933dd6..1a0645efa92 100644
--- a/be/src/storage/segment/vertical_segment_writer.h
+++ b/be/src/storage/segment/vertical_segment_writer.h
@@ -199,8 +199,12 @@ private:
Status _check_column_writer_disk_capacity(size_t cid);
Status _finalize_column_writer_and_update_meta(size_t cid);
- bool _is_mow();
- bool _is_mow_with_cluster_key();
+ bool _is_mow() {
+ return _tablet_schema->keys_type() == UNIQUE_KEYS &&
_opts.enable_unique_key_merge_on_write;
+ }
+ bool _is_mow_with_cluster_key() {
+ return _is_mow() && !_tablet_schema->cluster_key_uids().empty();
+ }
private:
friend class ::doris::BlockAggregator;
diff --git a/be/test/storage/segment/segment_writer_mow_check_test.cpp
b/be/test/storage/segment/segment_writer_mow_check_test.cpp
new file mode 100644
index 00000000000..e8be43a7fa4
--- /dev/null
+++ b/be/test/storage/segment/segment_writer_mow_check_test.cpp
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include <memory>
+#include <string>
+
+#include "io/fs/local_file_system.h"
+#include "storage/olap_common.h"
+#include "storage/rowset/rowset_id_generator.h"
+#include "storage/segment/segment_writer.h"
+#include "storage/segment/vertical_segment_writer.h"
+#include "storage/tablet/tablet_schema.h"
+
+namespace doris::segment_v2 {
+
+// Subclass in a different translation unit that calls _is_mow() and
_is_mow_with_cluster_key().
+// This test verifies that these inline private methods are visible across
translation units.
+// Previously they were defined with `inline` in .cpp files, which caused
linker errors
+// when compiled with -O1 or higher (TSAN/RELEASE), because the compiler
inlined them
+// and did not export the symbols.
+class TestSegmentWriterMowCheck : public SegmentWriter {
+public:
+ using SegmentWriter::SegmentWriter;
+
+ bool check_is_mow() { return _is_mow(); }
+ bool check_is_mow_with_cluster_key() { return _is_mow_with_cluster_key(); }
+};
+
+class TestVerticalSegmentWriterMowCheck : public VerticalSegmentWriter {
+public:
+ using VerticalSegmentWriter::VerticalSegmentWriter;
+
+ bool check_is_mow() { return _is_mow(); }
+ bool check_is_mow_with_cluster_key() { return _is_mow_with_cluster_key(); }
+};
+
+static const std::string kSegmentDir =
"./ut_dir/segment_writer_mow_check_test";
+
+TabletColumnPtr create_int_key(int32_t id) {
+ auto column = std::make_shared<TabletColumn>();
+ column->_unique_id = id;
+ column->_col_name = std::to_string(id);
+ column->_type = FieldType::OLAP_FIELD_TYPE_INT;
+ column->_is_key = true;
+ column->_is_nullable = false;
+ column->_length = 4;
+ column->_index_length = 4;
+ return column;
+}
+
+TabletColumnPtr create_int_value(int32_t id) {
+ auto column = std::make_shared<TabletColumn>();
+ column->_unique_id = id;
+ column->_col_name = std::to_string(id);
+ column->_type = FieldType::OLAP_FIELD_TYPE_INT;
+ column->_is_key = false;
+ column->_is_nullable = true;
+ column->_length = 4;
+ column->_index_length = 4;
+ return column;
+}
+
+TabletSchemaSPtr create_unique_key_schema() {
+ TabletSchemaSPtr schema = std::make_shared<TabletSchema>();
+ schema->append_column(*create_int_key(0));
+ schema->append_column(*create_int_value(1));
+ schema->_keys_type = UNIQUE_KEYS;
+ return schema;
+}
+
+TabletSchemaSPtr create_dup_key_schema() {
+ TabletSchemaSPtr schema = std::make_shared<TabletSchema>();
+ schema->append_column(*create_int_key(0));
+ schema->append_column(*create_int_value(1));
+ schema->_keys_type = DUP_KEYS;
+ return schema;
+}
+
+TabletSchemaSPtr create_unique_key_schema_with_cluster_key() {
+ auto schema = create_unique_key_schema();
+ schema->_cluster_key_uids = {1};
+ return schema;
+}
+
+class SegmentWriterMowCheckTest : public testing::Test {
+public:
+ void SetUp() override {
+ auto fs = io::global_local_filesystem();
+ auto st = fs->delete_directory(kSegmentDir);
+ ASSERT_TRUE(st.ok() || st.is<ErrorCode::NOT_FOUND>()) << st;
+ st = fs->create_directory(kSegmentDir);
+ ASSERT_TRUE(st.ok()) << st;
+ }
+
+ void TearDown() override {
+
EXPECT_TRUE(io::global_local_filesystem()->delete_directory(kSegmentDir).ok());
+ }
+
+ io::FileWriterPtr create_file_writer(size_t segment_id) {
+ RowsetId rowset_id;
+ rowset_id.init(1);
+ std::string filename = fmt::format("{}_{}.dat", rowset_id.to_string(),
segment_id);
+ std::string path = fmt::format("{}/{}", kSegmentDir, filename);
+ io::FileWriterPtr file_writer;
+ auto st = io::global_local_filesystem()->create_file(path,
&file_writer);
+ EXPECT_TRUE(st.ok()) << st;
+ return file_writer;
+ }
+};
+
+TEST_F(SegmentWriterMowCheckTest, segment_writer_is_mow_false_for_dup_key) {
+ auto schema = create_dup_key_schema();
+ SegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(0);
+ TestSegmentWriterMowCheck writer(file_writer.get(), 0, schema, nullptr,
nullptr, opts, nullptr);
+ EXPECT_FALSE(writer.check_is_mow());
+ EXPECT_FALSE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest, segment_writer_is_mow_true_for_unique_mow) {
+ auto schema = create_unique_key_schema();
+ SegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(1);
+ TestSegmentWriterMowCheck writer(file_writer.get(), 1, schema, nullptr,
nullptr, opts, nullptr);
+ EXPECT_TRUE(writer.check_is_mow());
+ EXPECT_FALSE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest,
segment_writer_is_mow_false_when_mow_disabled) {
+ auto schema = create_unique_key_schema();
+ SegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = false;
+ auto file_writer = create_file_writer(2);
+ TestSegmentWriterMowCheck writer(file_writer.get(), 2, schema, nullptr,
nullptr, opts, nullptr);
+ EXPECT_FALSE(writer.check_is_mow());
+ EXPECT_FALSE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest, segment_writer_is_mow_with_cluster_key) {
+ auto schema = create_unique_key_schema_with_cluster_key();
+ SegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(3);
+ TestSegmentWriterMowCheck writer(file_writer.get(), 3, schema, nullptr,
nullptr, opts, nullptr);
+ EXPECT_TRUE(writer.check_is_mow());
+ EXPECT_TRUE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest,
vertical_segment_writer_is_mow_false_for_dup_key) {
+ auto schema = create_dup_key_schema();
+ VerticalSegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(4);
+ TestVerticalSegmentWriterMowCheck writer(file_writer.get(), 4, schema,
nullptr, nullptr, opts,
+ nullptr);
+ EXPECT_FALSE(writer.check_is_mow());
+ EXPECT_FALSE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest,
vertical_segment_writer_is_mow_true_for_unique_mow) {
+ auto schema = create_unique_key_schema();
+ VerticalSegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(5);
+ TestVerticalSegmentWriterMowCheck writer(file_writer.get(), 5, schema,
nullptr, nullptr, opts,
+ nullptr);
+ EXPECT_TRUE(writer.check_is_mow());
+ EXPECT_FALSE(writer.check_is_mow_with_cluster_key());
+}
+
+TEST_F(SegmentWriterMowCheckTest,
vertical_segment_writer_is_mow_with_cluster_key) {
+ auto schema = create_unique_key_schema_with_cluster_key();
+ VerticalSegmentWriterOptions opts;
+ opts.enable_unique_key_merge_on_write = true;
+ auto file_writer = create_file_writer(6);
+ TestVerticalSegmentWriterMowCheck writer(file_writer.get(), 6, schema,
nullptr, nullptr, opts,
+ nullptr);
+ EXPECT_TRUE(writer.check_is_mow());
+ EXPECT_TRUE(writer.check_is_mow_with_cluster_key());
+}
+
+} // namespace doris::segment_v2
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
