Copilot commented on code in PR #67360:
URL: https://github.com/apache/doris/pull/67360#discussion_r3901434965
##########
be/src/storage/merger.h:
##########
@@ -48,6 +50,13 @@ using VerticalCompactionProgressCallback =
class Merger {
public:
+ struct KeyRange {
+ std::optional<OlapTuple> lower_key;
+ std::optional<OlapTuple> upper_key;
+ bool lower_inclusive;
+ bool upper_inclusive;
+ };
Review Comment:
`Merger::KeyRange` leaves `lower_inclusive` / `upper_inclusive`
uninitialized unless every caller explicitly sets them, which is undefined
behavior and easy to misuse as the API is public. Give these members default
initializers (matching `TabletReader::ReaderParams` defaults) so a
partially-initialized `KeyRange` is still deterministic.
##########
be/src/storage/merger.cpp:
##########
@@ -64,11 +66,27 @@
#include "util/slice.h"
namespace doris {
+
+namespace {
+
+void set_key_range(TabletReader::ReaderParams* reader_params, const
Merger::KeyRange& range) {
+ if (!range.lower_key.has_value() && !range.upper_key.has_value()) {
+ return;
+ }
+ reader_params->start_key.push_back(range.lower_key);
+ reader_params->end_key.push_back(range.upper_key);
+ reader_params->start_key_include = range.lower_inclusive;
+ reader_params->end_key_include = range.upper_inclusive;
+}
Review Comment:
`set_key_range()` silently ignores a `KeyRange` with both bounds unset.
Since callers can represent “no range” via `std::nullopt`, accepting an
all-unbounded `KeyRange` here can mask bugs (and in `vertical_merge_rowsets` it
also changes behavior via `key_range.has_value()` checks). Fail fast when both
bounds are absent.
##########
be/test/storage/tablet_reader_test.cpp:
##########
@@ -135,4 +135,26 @@ TEST_F(TabletReaderTest,
remove_delete_columns_keeps_unrelated_paths) {
EXPECT_EQ(size_t(2), access_paths.size());
}
+
+TEST_F(TabletReaderTest, initializes_unbounded_key_ranges) {
+ auto schema = create_schema({{"k1", 10}});
+ OlapTuple upper_key;
+ upper_key.add_field(Field::create_field<TYPE_INT>(10));
+ OlapTuple lower_key;
+ lower_key.add_field(Field::create_field<TYPE_INT>(20));
+
+ TabletReader::ReaderParams params;
+ params.start_key.emplace_back(std::nullopt);
+ params.end_key.emplace_back(std::move(upper_key));
+ params.start_key.emplace_back(std::move(lower_key));
+ params.end_key.emplace_back(std::nullopt);
+
+ TabletReader reader;
+ reader._tablet_schema = std::move(schema);
+ ASSERT_TRUE(reader._init_keys_param(params).ok());
+ EXPECT_FALSE(reader._keys_param.start_keys[0].has_value());
+ EXPECT_TRUE(reader._keys_param.end_keys[0].has_value());
+ EXPECT_TRUE(reader._keys_param.start_keys[1].has_value());
+ EXPECT_FALSE(reader._keys_param.end_keys[1].has_value());
Review Comment:
This test accesses `TabletReader` protected members (`_tablet_schema`,
`_init_keys_param`, `_keys_param`) directly, which won’t compile unless
visibility is intentionally widened. A common pattern in this repo is to create
a small derived helper that exposes the needed members via `using` declarations.
--
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]
