HappenLee commented on code in PR #67817:
URL: https://github.com/apache/doris/pull/67817#discussion_r4080995460
##########
be/test/storage/segment/column_reader_test.cpp:
##########
@@ -450,6 +474,370 @@ TEST_F(ColumnReaderTest,
NullMapOnlyReadBySparseRowidsAcrossPages) {
EXPECT_EQ(2, nullable_col.get_nested_column().size());
}
+TEST_F(ColumnReaderTest, ArrayReadByRowidsMatchesSequentialReadAcrossPages) {
+ // The generated offsets and non-null INT items both exceed the default 64
KiB data-page
+ // size, so the selected rowids exercise page transitions in both child
streams.
+ constexpr size_t num_rows = 12000;
+ constexpr size_t max_items_per_row = 3;
+ ColumnMetaPB meta;
+ TabletColumn
array_column(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE,
+ FieldType::OLAP_FIELD_TYPE_ARRAY);
+ TabletColumn
item_column(FieldAggregationMethod::OLAP_FIELD_AGGREGATION_NONE,
+ FieldType::OLAP_FIELD_TYPE_INT, true);
+ array_column.add_sub_column(item_column);
+
+ std::vector<int32_t> item_values;
+ std::vector<uint8_t> item_null_map;
+ std::vector<uint64_t> array_offsets(num_rows + 1, 0);
+ std::vector<uint8_t> array_null_map(num_rows, 0);
+ item_values.reserve(num_rows * max_items_per_row);
+ item_null_map.reserve(num_rows * max_items_per_row);
+ for (size_t row = 0; row < num_rows; ++row) {
+ const bool is_null = row % 7 == 1;
+ array_null_map[row] = is_null;
+ const size_t item_count = row % 11 == 0 ? 0 : (is_null ? 3 : row % 3 +
1);
+ for (size_t item = 0; item < item_count; ++item) {
+ item_values.push_back(static_cast<int32_t>(row * 10 + item));
+ item_null_map.push_back((row + item) % 5 == 0);
+ }
+ array_offsets[row + 1] = item_values.size();
+ }
+
+ const std::string file_name = COLUMN_READER_FILE_TEST_DIR +
"/array_read_by_rowids";
+ auto fs = io::global_local_filesystem();
+ {
+ io::FileWriterPtr file_writer;
+ auto st = fs->create_file(file_name, &file_writer);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+
+ ColumnWriterOptions writer_options;
+ writer_options.meta = &meta;
+ writer_options.meta->set_column_id(0);
+ writer_options.meta->set_unique_id(0);
+
writer_options.meta->set_type(static_cast<int32_t>(FieldType::OLAP_FIELD_TYPE_ARRAY));
+ writer_options.meta->set_length(0);
+ writer_options.meta->set_encoding(DEFAULT_ENCODING);
+ writer_options.meta->set_compression(CompressionTypePB::LZ4F);
+ writer_options.meta->set_is_nullable(true);
+
+ auto* child_meta = meta.add_children_columns();
+ child_meta->set_column_id(1);
+ child_meta->set_unique_id(1);
+
child_meta->set_type(static_cast<int32_t>(FieldType::OLAP_FIELD_TYPE_INT));
+ child_meta->set_length(0);
+ child_meta->set_encoding(BIT_SHUFFLE);
+ child_meta->set_compression(CompressionTypePB::LZ4F);
+ child_meta->set_is_nullable(true);
+
+ std::unique_ptr<ColumnWriter> writer;
+ st = ColumnWriter::create(writer_options, &array_column,
file_writer.get(), &writer);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ ASSERT_TRUE(writer->init().ok());
+ // Use the vectorized append path so even null parent rows keep their
physical item span.
+ const std::array<uint64_t, 4> array_data
{static_cast<uint64_t>(item_values.size()),
+
reinterpret_cast<uint64_t>(array_offsets.data()),
+
reinterpret_cast<uint64_t>(item_values.data()),
+
reinterpret_cast<uint64_t>(item_null_map.data())};
+ ASSERT_TRUE(writer->append(array_null_map.data(), array_data.data(),
num_rows).ok());
+ ASSERT_TRUE(writer->finish().ok());
+ ASSERT_TRUE(writer->write_data().ok());
+ ASSERT_TRUE(writer->write_ordinal_index().ok());
+ ASSERT_TRUE(file_writer->close().ok());
+ }
+
+ io::FileReaderSPtr file_reader;
+ auto st = fs->open_file(file_name, &file_reader);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ ColumnReaderOptions reader_options;
+ std::shared_ptr<ColumnReader> reader;
+ st = ColumnReader::create(reader_options, meta, num_rows, file_reader,
&reader);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+
+ DataTypePtr array_type = std::make_shared<DataTypeArray>(
+
std::make_shared<DataTypeNullable>(std::make_shared<DataTypeInt32>()));
+ DataTypePtr column_type = std::make_shared<DataTypeNullable>(array_type);
+ auto create_iterator = [&](OlapReaderStatistics* stats,
+ ColumnIteratorUPtr* iterator) -> Status {
+ RETURN_IF_ERROR(reader->new_iterator(iterator, &array_column));
+ ColumnIteratorOptions options;
+ options.stats = stats;
+ options.file_reader = file_reader.get();
+ return (*iterator)->init(options);
+ };
+
+ MutableColumnPtr baseline = column_type->create_column();
+ {
+ ColumnIteratorUPtr iterator;
+ OlapReaderStatistics stats;
+ st = create_iterator(&stats, &iterator);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ ASSERT_TRUE(iterator->seek_to_ordinal(0).ok());
+ size_t rows_to_read = num_rows;
+ bool has_null = false;
+ st = iterator->next_batch(&rows_to_read, baseline, &has_null);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ ASSERT_EQ(num_rows, rows_to_read);
+ }
+
+ std::vector<rowid_t> rowids {0, 1, 2, 7, 8, 11, 4095, 4096, 8191, 8192};
+ for (rowid_t rowid = 10000; rowid <= 11000; ++rowid) {
+ rowids.push_back(rowid);
+ }
+ rowids.push_back(11998);
+ rowids.push_back(11999);
+ MutableColumnPtr actual = column_type->create_column();
+ {
+ ColumnIteratorUPtr iterator;
+ OlapReaderStatistics stats;
+ st = create_iterator(&stats, &iterator);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ st = iterator->read_by_rowids(rowids.data(), rowids.size(), actual);
+ ASSERT_TRUE(st.ok()) << st.to_string();
+ }
+
+ ASSERT_EQ(1, array_null_map[1]);
+ ASSERT_GT(array_offsets[2] - array_offsets[1], 0);
+ const auto& baseline_nullable = assert_cast<const
ColumnNullable&>(*baseline);
+ const auto& baseline_array = assert_cast<const ColumnArray&,
TypeCheckOnRelease::DISABLE>(
+ baseline_nullable.get_nested_column());
+ ASSERT_EQ(1, baseline_nullable.get_null_map_data()[1]);
+ ASSERT_GT(baseline_array.size_at(1), 0);
+
+ // Compare physical item spans as well as logical rows, including payload
under NULL parents.
+ auto check_selected_rows = [&](const std::vector<rowid_t>& selected_rowids,
+ const IColumn& result) {
+ ASSERT_EQ(selected_rowids.size(), result.size());
+ const auto& actual_nullable = assert_cast<const
ColumnNullable&>(result);
+ const auto& actual_array = assert_cast<const ColumnArray&,
TypeCheckOnRelease::DISABLE>(
+ actual_nullable.get_nested_column());
+ size_t expected_item_count = 0;
+ size_t actual_item_index = 0;
+ for (size_t i = 0; i < selected_rowids.size(); ++i) {
+ const auto rowid = selected_rowids[i];
+ EXPECT_EQ(baseline_nullable.get_null_map_data()[rowid],
+ actual_nullable.get_null_map_data()[i]);
+
+ const size_t row_item_count = baseline_array.size_at(rowid);
+ expected_item_count += row_item_count;
+ EXPECT_EQ(expected_item_count, actual_array.get_offsets()[i]);
+ for (size_t item = 0; item < row_item_count; ++item) {
+ EXPECT_EQ(0, actual_array.get_data().compare_at(
+ actual_item_index,
baseline_array.offset_at(rowid) + item,
+ baseline_array.get_data(), 1));
+ ++actual_item_index;
+ }
+ }
+ EXPECT_EQ(expected_item_count, actual_array.get_data().size());
+ };
+ check_selected_rows(rowids, *actual);
+
+ {
+ // FixedReadPlan preserves input order. Read the final physical row
before an earlier row
+ // to cover the unordered fallback, including the end-of-offset-stream
sentinel.
+ const std::vector<rowid_t> unordered_rowids {11999, 0};
Review Comment:
Please delete this unordered-input subcase (the block using
`unordered_rowids {11999, 0}`, including its `FixedReadPlan` comment) when
replacing the fallback with `DCHECK`.
This directly constructs input that violates the ordered-row-ID
precondition; it bypasses the ordering guarantees of the production callers
described in [the related review
comment](https://github.com/apache/doris/pull/67817#discussion_r4080986400).
Requiring a successful read here would preserve the unnecessary fallback
behavior.
Keep the surrounding ordered cross-page, last-row, and lazy-materialization
coverage.
--
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]
