Copilot commented on code in PR #61844:
URL: https://github.com/apache/doris/pull/61844#discussion_r3004433442
##########
be/src/storage/segment/lazy_init_segment_iterator.cpp:
##########
@@ -40,8 +40,14 @@ Status LazyInitSegmentIterator::init(const
StorageReadOptions& opts) {
std::shared_ptr<Segment> segment;
{
SegmentCacheHandle segment_cache_handle;
- RETURN_IF_ERROR(SegmentLoader::instance()->load_segment(
- _rowset, _segment_id, &segment_cache_handle,
_should_use_cache, false, opts.stats));
+ auto st = SegmentLoader::instance()->load_segment(
+ _rowset, _segment_id, &segment_cache_handle,
_should_use_cache, false, opts.stats);
+ if (st.is<ErrorCode::NOT_FOUND>() && config::ignore_not_found_segment)
{
+ LOG(WARNING) << "segment not found, skip it. seg_id=" <<
_segment_id;
+ // _inner_iterator remains nullptr, next_batch() will return EOF
+ return Status::OK();
Review Comment:
`LazyInitSegmentIterator::init()` logs only `seg_id` when skipping
NOT_FOUND, which makes correlating the warning to a specific tablet/rowset
difficult in production. Please include at least the rowset id (and ideally
tablet id / segment path if available) in the warning, and consider
rate-limiting if this can be hit repeatedly.
##########
be/test/storage/segment/ignore_not_found_segment_test.cpp:
##########
@@ -0,0 +1,244 @@
+// 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 "common/config.h"
+#include "common/status.h"
+#include "json2pb/json_to_pb.h"
+#include "runtime/exec_env.h"
+#include "storage/rowset/beta_rowset.h"
+#include "storage/segment/lazy_init_segment_iterator.h"
+#include "storage/segment/segment_loader.h"
+#include "util/debug_points.h"
+
+namespace doris {
+
+class IgnoreNotFoundSegmentTest : public testing::Test {
+protected:
+ void SetUp() override {
+ _saved_ignore = config::ignore_not_found_segment;
+ _saved_debug_points = config::enable_debug_points;
+ config::enable_debug_points = true;
+
+ // Set up a SegmentLoader for LazyInitSegmentIterator tests
+ _saved_segment_loader = ExecEnv::GetInstance()->segment_loader();
+ _segment_loader = new SegmentLoader(1024 * 1024, 100);
+ ExecEnv::GetInstance()->set_segment_loader(_segment_loader);
+ }
+
+ void TearDown() override {
+ DebugPoints::instance()->clear();
+ config::ignore_not_found_segment = _saved_ignore;
+ config::enable_debug_points = _saved_debug_points;
+
+ ExecEnv::GetInstance()->set_segment_loader(_saved_segment_loader);
+ delete _segment_loader;
+ _segment_loader = nullptr;
Review Comment:
The test fixture uses `DebugPoints::instance()->clear()` in `TearDown()`,
which removes *all* debug points for the entire process. This can create
test-order coupling if other tests in the same binary rely on debug points.
Prefer removing only the points added by this test (e.g.,
`remove("BetaRowset::load_segment.return_not_found")`) or using an RAII helper
that adds/removes a named debug point and restores
`config::enable_debug_points`.
##########
be/src/storage/rowset/beta_rowset.cpp:
##########
@@ -251,7 +251,14 @@ Status BetaRowset::load_segments(int64_t seg_id_begin,
int64_t seg_id_end,
int64_t seg_id = seg_id_begin;
while (seg_id < seg_id_end) {
std::shared_ptr<segment_v2::Segment> segment;
- RETURN_IF_ERROR(load_segment(seg_id, nullptr, &segment));
+ auto st = load_segment(seg_id, nullptr, &segment);
+ if (st.is<ErrorCode::NOT_FOUND>() && config::ignore_not_found_segment)
{
+ LOG(WARNING) << "segment not found, skip it. rowset_id=" <<
rowset_id()
+ << ", seg_id=" << seg_id;
+ seg_id++;
+ continue;
Review Comment:
Like `SegmentLoader`, `BetaRowset::load_segments()` now logs a WARNING per
missing segment. If a rowset has many missing segments (or the scan is
retried), this can generate a large volume of logs. Consider rate limiting
and/or logging a summary once per rowset (e.g., number of skipped segments) to
reduce operational noise.
##########
be/src/storage/segment/segment_loader.cpp:
##########
@@ -91,8 +91,14 @@ Status SegmentLoader::load_segments(const
BetaRowsetSharedPtr& rowset,
return Status::OK();
}
for (int64_t i = 0; i < rowset->num_segments(); i++) {
- RETURN_IF_ERROR(load_segment(rowset, i, cache_handle, use_cache,
need_load_pk_index_and_bf,
- index_load_stats));
+ auto st = load_segment(rowset, i, cache_handle, use_cache,
need_load_pk_index_and_bf,
+ index_load_stats);
+ if (st.is<ErrorCode::NOT_FOUND>() && config::ignore_not_found_segment)
{
+ LOG(WARNING) << "segment not found, skip it. rowset_id=" <<
rowset->rowset_id()
+ << ", seg_id=" << i;
+ continue;
+ }
Review Comment:
`SegmentLoader::load_segments()` now skips NOT_FOUND segments and still
returns OK, but many call sites implicitly assume
`cache_handle->get_segments()` is indexed by `seg_id` and has
`size()==rowset->num_segments()`. For example, `BaseTablet::lookup_row_key()`
indexes `segments[id]` where `id` is a seg_id; with skipped entries this can
become out-of-bounds or dereference the wrong segment in release builds. To
make skipping safe, either (a) preserve the seg_id->index contract by resizing
the segment vector to `num_segments` and storing loaded segments at
`segments[seg_id]` (leaving nullptr for missing) and update callers to handle
nullptr, or (b) change the API to return a mapping and update all callers to
lookup by `Segment::id()` instead of positional indexing.
```suggestion
```
##########
be/test/storage/segment/ignore_not_found_segment_test.cpp:
##########
@@ -0,0 +1,244 @@
+// 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 "common/config.h"
+#include "common/status.h"
+#include "json2pb/json_to_pb.h"
+#include "runtime/exec_env.h"
+#include "storage/rowset/beta_rowset.h"
+#include "storage/segment/lazy_init_segment_iterator.h"
+#include "storage/segment/segment_loader.h"
+#include "util/debug_points.h"
+
+namespace doris {
+
+class IgnoreNotFoundSegmentTest : public testing::Test {
+protected:
+ void SetUp() override {
+ _saved_ignore = config::ignore_not_found_segment;
+ _saved_debug_points = config::enable_debug_points;
+ config::enable_debug_points = true;
+
+ // Set up a SegmentLoader for LazyInitSegmentIterator tests
+ _saved_segment_loader = ExecEnv::GetInstance()->segment_loader();
+ _segment_loader = new SegmentLoader(1024 * 1024, 100);
+ ExecEnv::GetInstance()->set_segment_loader(_segment_loader);
+ }
+
+ void TearDown() override {
+ DebugPoints::instance()->clear();
+ config::ignore_not_found_segment = _saved_ignore;
+ config::enable_debug_points = _saved_debug_points;
+
+ ExecEnv::GetInstance()->set_segment_loader(_saved_segment_loader);
+ delete _segment_loader;
+ _segment_loader = nullptr;
+ }
+
+ BetaRowsetSharedPtr create_rowset(int num_segments) {
+ auto schema = std::make_shared<TabletSchema>();
+ TabletColumn col;
+ col.set_name("c1");
+ col.set_unique_id(0);
+ col.set_type(FieldType::OLAP_FIELD_TYPE_INT);
+ col.set_length(4);
+ col.set_is_key(true);
+ col.set_is_nullable(false);
+ schema->append_column(col);
+ schema->_keys_type = DUP_KEYS;
+
+ auto rsm = std::make_shared<RowsetMeta>();
+ std::string json = R"({
+ "rowset_id": 540081,
+ "tablet_id": 10001,
+ "partition_id": 10000,
+ "tablet_schema_hash": 567997577,
+ "rowset_type": "BETA_ROWSET",
+ "rowset_state": "VISIBLE",
+ "empty": false
+ })";
+ RowsetMetaPB pb;
+ json2pb::JsonToProtoMessage(json, &pb);
+ pb.set_start_version(0);
+ pb.set_end_version(1);
+ pb.set_num_segments(num_segments);
+ rsm->init_from_pb(pb);
Review Comment:
The return value from `json2pb::JsonToProtoMessage()` is ignored here. Other
tests in the repo treat this as a `bool` and assert success; if the JSON format
changes, silently proceeding can make failures harder to diagnose. Please
capture the return value and `ASSERT_TRUE/EXPECT_TRUE` it (or use a helper that
returns `Status`).
##########
be/test/storage/segment/ignore_not_found_segment_test.cpp:
##########
@@ -0,0 +1,244 @@
+// 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 "common/config.h"
+#include "common/status.h"
+#include "json2pb/json_to_pb.h"
+#include "runtime/exec_env.h"
+#include "storage/rowset/beta_rowset.h"
+#include "storage/segment/lazy_init_segment_iterator.h"
+#include "storage/segment/segment_loader.h"
+#include "util/debug_points.h"
+
+namespace doris {
+
+class IgnoreNotFoundSegmentTest : public testing::Test {
+protected:
+ void SetUp() override {
+ _saved_ignore = config::ignore_not_found_segment;
+ _saved_debug_points = config::enable_debug_points;
+ config::enable_debug_points = true;
+
+ // Set up a SegmentLoader for LazyInitSegmentIterator tests
+ _saved_segment_loader = ExecEnv::GetInstance()->segment_loader();
+ _segment_loader = new SegmentLoader(1024 * 1024, 100);
+ ExecEnv::GetInstance()->set_segment_loader(_segment_loader);
+ }
+
+ void TearDown() override {
+ DebugPoints::instance()->clear();
+ config::ignore_not_found_segment = _saved_ignore;
+ config::enable_debug_points = _saved_debug_points;
+
+ ExecEnv::GetInstance()->set_segment_loader(_saved_segment_loader);
+ delete _segment_loader;
+ _segment_loader = nullptr;
+ }
+
+ BetaRowsetSharedPtr create_rowset(int num_segments) {
+ auto schema = std::make_shared<TabletSchema>();
+ TabletColumn col;
+ col.set_name("c1");
+ col.set_unique_id(0);
+ col.set_type(FieldType::OLAP_FIELD_TYPE_INT);
+ col.set_length(4);
+ col.set_is_key(true);
+ col.set_is_nullable(false);
+ schema->append_column(col);
+ schema->_keys_type = DUP_KEYS;
+
+ auto rsm = std::make_shared<RowsetMeta>();
+ std::string json = R"({
+ "rowset_id": 540081,
+ "tablet_id": 10001,
+ "partition_id": 10000,
+ "tablet_schema_hash": 567997577,
+ "rowset_type": "BETA_ROWSET",
+ "rowset_state": "VISIBLE",
+ "empty": false
+ })";
+ RowsetMetaPB pb;
+ json2pb::JsonToProtoMessage(json, &pb);
+ pb.set_start_version(0);
+ pb.set_end_version(1);
+ pb.set_num_segments(num_segments);
+ rsm->init_from_pb(pb);
+ rsm->set_tablet_schema(schema);
+
+ return std::make_shared<BetaRowset>(schema, rsm, "");
+ }
+
+ bool _saved_ignore = true;
+ bool _saved_debug_points = false;
+ SegmentLoader* _saved_segment_loader = nullptr;
+ SegmentLoader* _segment_loader = nullptr;
+};
+
+// Test: BetaRowset::load_segments skips NOT_FOUND segments when config enabled
+TEST_F(IgnoreNotFoundSegmentTest, BetaRowsetLoadSegmentsSkipsNotFound) {
+ config::ignore_not_found_segment = true;
+ auto rowset = create_rowset(3);
+
+ DebugPoints::instance()->add("BetaRowset::load_segment.return_not_found");
+
+ std::vector<segment_v2::SegmentSharedPtr> segments;
+ auto st = rowset->load_segments(&segments);
+ // All segments are "not found" but should be skipped, resulting in OK
with empty segments
+ ASSERT_TRUE(st.ok()) << st;
+ ASSERT_EQ(0, segments.size());
+}
+
+// Test: BetaRowset::load_segments fails on NOT_FOUND when config disabled
+TEST_F(IgnoreNotFoundSegmentTest,
BetaRowsetLoadSegmentsFailsWhenConfigDisabled) {
+ config::ignore_not_found_segment = false;
+ auto rowset = create_rowset(3);
+
+ DebugPoints::instance()->add("BetaRowset::load_segment.return_not_found");
+
+ std::vector<segment_v2::SegmentSharedPtr> segments;
+ auto st = rowset->load_segments(&segments);
+ ASSERT_TRUE(st.is<ErrorCode::NOT_FOUND>()) << st;
+ ASSERT_EQ(0, segments.size());
+}
+
+// Test: BetaRowset::load_segments with range skips NOT_FOUND
+TEST_F(IgnoreNotFoundSegmentTest, BetaRowsetLoadSegmentsRangeSkipsNotFound) {
+ config::ignore_not_found_segment = true;
+ auto rowset = create_rowset(5);
+
+ DebugPoints::instance()->add("BetaRowset::load_segment.return_not_found");
+
+ std::vector<segment_v2::SegmentSharedPtr> segments;
+ auto st = rowset->load_segments(1, 4, &segments);
+ ASSERT_TRUE(st.ok()) << st;
+ ASSERT_EQ(0, segments.size());
+}
+
+// Test: SegmentLoader::load_segments skips NOT_FOUND segments when config
enabled
+TEST_F(IgnoreNotFoundSegmentTest, SegmentLoaderLoadSegmentsSkipsNotFound) {
+ config::ignore_not_found_segment = true;
+ auto rowset = create_rowset(3);
+
+ DebugPoints::instance()->add("BetaRowset::load_segment.return_not_found");
+
+ // Create a SegmentLoader with a small cache
+ SegmentLoader loader(1024 * 1024, 100);
+ SegmentCacheHandle handle;
+ auto st = loader.load_segments(rowset, &handle, false);
+ ASSERT_TRUE(st.ok()) << st;
+ ASSERT_EQ(0, handle.get_segments().size());
+ ASSERT_TRUE(handle.is_inited());
+}
+
Review Comment:
The new UTs validate that `SegmentLoader::load_segments()` returns OK when
segments are missing, but they don't cover any real caller that relies on
`SegmentCacheHandle::get_segments()` being indexable by `seg_id` (e.g., code
paths that do `segments[seg_id]`). Given the change in semantics, please add a
regression test exercising such a caller (or explicitly validate the
seg_id->vector-index contract you intend to provide).
```suggestion
// Regression test: validate that SegmentCacheHandle::get_segments() can be
indexed by seg_id
// after a successful SegmentLoader::load_segments() call. This mimics real
callers that do
// `segments[seg_id]` and relies on the seg_id -> vector-index contract.
TEST_F(IgnoreNotFoundSegmentTest, SegmentLoaderSegmentsIndexableBySegId) {
config::ignore_not_found_segment = true;
auto rowset = create_rowset(3);
// Do not inject NOT_FOUND for this test; we want all segments to load
successfully
SegmentLoader loader(1024 * 1024, 100);
SegmentCacheHandle handle;
auto st = loader.load_segments(rowset, &handle, false);
ASSERT_TRUE(st.ok()) << st;
ASSERT_TRUE(handle.is_inited());
const auto& segments = handle.get_segments();
// Expect that we can index by seg_id in [0, 3)
ASSERT_GE(segments.size(), 3);
for (int seg_id = 0; seg_id < 3; ++seg_id) {
// Real callers rely on segments[seg_id] being valid for each seg_id
ASSERT_NE(nullptr, segments[seg_id]);
}
}
```
##########
be/src/storage/segment/lazy_init_segment_iterator.h:
##########
@@ -17,6 +17,7 @@
#pragma once
+#include "common/config.h"
Review Comment:
`lazy_init_segment_iterator.h` adds `#include "common/config.h"`, but this
header doesn't reference `config::` anywhere. Keeping this include in a
widely-used header increases compile-time dependencies; it should be removed
(the `.cpp` already includes what it needs transitively).
```suggestion
```
##########
be/src/storage/segment/segment_loader.cpp:
##########
@@ -91,8 +91,14 @@ Status SegmentLoader::load_segments(const
BetaRowsetSharedPtr& rowset,
return Status::OK();
}
for (int64_t i = 0; i < rowset->num_segments(); i++) {
- RETURN_IF_ERROR(load_segment(rowset, i, cache_handle, use_cache,
need_load_pk_index_and_bf,
- index_load_stats));
+ auto st = load_segment(rowset, i, cache_handle, use_cache,
need_load_pk_index_and_bf,
+ index_load_stats);
+ if (st.is<ErrorCode::NOT_FOUND>() && config::ignore_not_found_segment)
{
+ LOG(WARNING) << "segment not found, skip it. rowset_id=" <<
rowset->rowset_id()
+ << ", seg_id=" << i;
+ continue;
Review Comment:
The warning log inside the segment-load loop can spam logs when multiple
segments are missing (and `load_segments()` is called frequently during query
execution). Consider rate limiting (e.g., `LOG_EVERY_N` /
`LOG_EVERY_N_SECONDS`) and/or aggregating counts per rowset to reduce
operational noise while still preserving debuggability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
