github-actions[bot] commented on code in PR #67360:
URL: https://github.com/apache/doris/pull/67360#discussion_r4023390729
##########
be/src/storage/tablet/tablet_reader.cpp:
##########
@@ -96,26 +96,27 @@ Status TabletReader::_capture_rs_readers(const
ReaderParams& read_params) {
bool is_lower_key_included = _keys_param.start_key_include;
bool is_upper_key_included = _keys_param.end_key_include;
+ DORIS_CHECK_EQ(_keys_param.start_keys.size(), _keys_param.end_keys.size());
for (int i = 0; i < _keys_param.start_keys.size(); ++i) {
- // lower bound
- RowCursor& start_key = _keys_param.start_keys[i];
- RowCursor& end_key = _keys_param.end_keys[i];
-
- if (!is_lower_key_included) {
- if (compare_row_key(start_key, end_key) >= 0) {
- VLOG_NOTICE << "return EOF when lower key not include"
- << ", start_key=" << start_key.to_string()
- << ", end_key=" << end_key.to_string();
- eof = true;
- break;
- }
- } else {
- if (compare_row_key(start_key, end_key) > 0) {
- VLOG_NOTICE << "return EOF when lower key include="
- << ", start_key=" << start_key.to_string()
- << ", end_key=" << end_key.to_string();
- eof = true;
- break;
+ const auto& start_key = _keys_param.start_keys[i];
+ const auto& end_key = _keys_param.end_keys[i];
+ if (start_key.has_value() && end_key.has_value()) {
Review Comment:
[P2] Define empty key-range handling at the Merger boundary
For equal bounds, this precheck returns `END_OF_FILE` for `(K,K]` and
`(K,K)` solely because the lower endpoint is exclusive, while `[K,K)` reaches
segment pruning and completes as an empty merge; `[K,K]` remains non-empty.
Both new Merger entry points propagate initialization EOF as a failure, so
callers cannot handle equivalent empty partitions consistently. Before the
range coordinator relies on this API, please validate reversed/equal bounds in
`set_key_range()` or normalize every empty interval to the same successful
no-op, and cover all four equal-bound inclusion shapes in horizontal and
vertical tests.
##########
be/test/storage/compaction/vertical_compaction_test.cpp:
##########
@@ -681,6 +686,155 @@ TEST_F(VerticalCompactionTest, TestDupKeyVerticalMerge) {
}
}
+TEST_F(VerticalCompactionTest, MergeHonorsRuntimeStateCancellation) {
+ constexpr int num_segments = 1;
+ std::vector<std::vector<std::vector<std::tuple<int64_t, int64_t>>>>
input_data;
+ generate_input_data(1, num_segments, 10, NONOVERLAPPING, input_data);
+ auto tablet_schema = create_schema();
+ auto input_rowset = create_rowset(tablet_schema, NONOVERLAPPING,
input_data.front(), 0);
+ auto tablet = create_tablet(*tablet_schema, false);
+
+ for (const bool is_vertical : {false, true}) {
+ RowsetReaderSharedPtr input_reader;
+ ASSERT_TRUE(input_rowset->create_reader(&input_reader).ok());
+ std::vector<RowsetReaderSharedPtr> input_readers =
{std::move(input_reader)};
+ auto writer_context =
+ create_rowset_writer_context(tablet_schema, NONOVERLAPPING,
3456, {0, 0});
+ auto writer_result =
+ RowsetFactory::create_rowset_writer(*engine_ref,
writer_context, is_vertical);
+ ASSERT_TRUE(writer_result.has_value()) << writer_result.error();
+ auto output_writer = std::move(writer_result).value();
+
+ RuntimeState runtime_state;
+ runtime_state.cancel(Status::Cancelled("injected compaction
cancellation"));
+ Merger::Statistics stats;
+ Status status;
+ if (is_vertical) {
+ status = Merger::vertical_merge_rowsets(
+ tablet, ReaderType::READER_BASE_COMPACTION,
*tablet_schema, input_readers,
+ output_writer.get(), 100, num_segments, &stats, nullptr,
std::nullopt,
+ std::nullopt, &runtime_state);
+ } else {
+ status = Merger::vmerge_rowsets(tablet,
ReaderType::READER_BASE_COMPACTION,
+ *tablet_schema, input_readers,
output_writer.get(),
+ &stats, std::nullopt,
std::nullopt, &runtime_state);
+ }
+ EXPECT_TRUE(status.is<ErrorCode::CANCELLED>()) << status;
+ }
+}
+
+TEST_F(VerticalCompactionTest, MergeHonorsKeyRanges) {
+ constexpr int num_rows = 10;
+ std::vector<std::vector<std::vector<std::tuple<int64_t, int64_t>>>>
input_data;
+ generate_input_data(1, 1, num_rows, NONOVERLAPPING, input_data);
+ auto tablet_schema = create_schema();
+ auto input_rowset = create_rowset(tablet_schema, NONOVERLAPPING,
input_data.front(), 0);
+ auto tablet = create_tablet(*tablet_schema, false);
+
+ auto make_key = [](int32_t value) {
+ OlapTuple key;
+ key.add_field(Field::create_field<TYPE_INT>(value));
+ return key;
+ };
+ auto run_case = [&](bool is_vertical, Merger::KeyRange key_range, int32_t
expected_begin,
+ int32_t expected_end) {
+ RowsetReaderSharedPtr input_reader;
+ ASSERT_TRUE(input_rowset->create_reader(&input_reader).ok());
+ std::vector<RowsetReaderSharedPtr> input_readers =
{std::move(input_reader)};
+ auto writer_context =
+ create_rowset_writer_context(tablet_schema, NONOVERLAPPING,
100, {0, 0});
+ auto writer_result =
+ RowsetFactory::create_rowset_writer(*engine_ref,
writer_context, is_vertical);
+ ASSERT_TRUE(writer_result.has_value()) << writer_result.error();
+ auto output_writer = std::move(writer_result).value();
+
+ Merger::Statistics stats;
+ RuntimeState runtime_state;
+ TQueryOptions query_options;
+ query_options.__set_enable_file_cache(false);
+ query_options.__set_disable_file_cache(false);
+ runtime_state.set_query_options(query_options);
Review Comment:
[P2] Keep cancellation state from selecting the compaction version
The `TQueryOptions` built here sets only cache flags, so its explicit IDL
default leaves `be_exec_version` at 0; `set_query_options()` replaces the
`RuntimeState` constructor's newest-version value with that 0. This state is
then forwarded beyond cancellation: `Segment::new_iterator()` and
`ReaderParams::get_be_exec_version()` use it instead of compaction's previous
newest-version fallback. On AGG_STATE columns whose nested functions reject old
versions (for example `percentile` or `window_funnel`), this makes the new
range API fail compatibility checks on current data. This remains after the
cache-policy fix. Please carry cancellation separately or keep non-query
readers on newest-version semantics, and cover an uncancelled AGG_STATE merge.
--
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]
