HappenLee commented on code in PR #67817:
URL: https://github.com/apache/doris/pull/67817#discussion_r4059743419
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -2432,11 +2438,187 @@ Status ArrayFileColumnIterator::read_by_rowids(const
rowid_t* rowids, const size
_recovery_from_place_holder_column(dst);
+ if (count == 0) {
+ return Status::OK();
+ }
+
+ // A null-only consumer needs one nested row per null marker, but no
lengths or item data.
+ if (read_null_map_only()) {
+ DORIS_CHECK(is_column_nullable(*dst));
+ auto& nullable_column = assert_cast<ColumnNullable&>(*dst);
+ if (_null_iterator) {
+ auto null_map_ptr = nullable_column.get_null_map_column_ptr();
+ MutableColumnPtr null_map_column = std::move(null_map_ptr);
+ RETURN_IF_ERROR(_null_iterator->read_by_rowids(rowids, count,
null_map_column));
+ } else {
+ // A nullable schema can read an old non-nullable segment, which
has no null stream.
+ nullable_column.get_null_map_column_ptr()->insert_many_vals(0,
count);
+ }
+ auto& column_array = assert_cast<ColumnArray&,
TypeCheckOnRelease::DISABLE>(
+ nullable_column.get_nested_column());
+ column_array.insert_many_defaults(count);
+ return Status::OK();
+ }
+
+ auto& column_array = assert_cast<ColumnArray&,
TypeCheckOnRelease::DISABLE>(
+ is_column_nullable(*dst) ?
static_cast<ColumnNullable&>(*dst).get_nested_column()
+ : *dst);
+ // The parent reader can stay active solely for lazy children. This flag
controls writing
+ // parent metadata to dst, not reading source offsets to locate those
children on disk.
+ const bool read_meta_columns = need_to_read_meta_columns();
+
+ if (_array_reader->is_nullable()) {
+ if (UNLIKELY(!is_column_nullable(*dst))) {
+ return Status::InternalError(
+ "unexpected non-nullable destination column for nullable
array reader");
+ }
+ auto& nullable_column = static_cast<ColumnNullable&>(*dst);
+ if (read_meta_columns) {
+ MutableColumnPtr null_map_column =
nullable_column.get_null_map_column_ptr();
+ RETURN_IF_ERROR(_null_iterator->read_by_rowids(rowids, count,
null_map_column));
+ } else {
+ DORIS_CHECK(nullable_column.get_null_map_column().size() == count);
+ }
+ } else if (read_meta_columns && is_column_nullable(*dst)) {
+
static_cast<ColumnNullable&>(*dst).get_null_map_column_ptr()->insert_many_vals(0,
count);
+ }
+
+ // Array row r spans [offset[r], offset[r + 1]) in the source item stream.
offset_rowids
+ // identifies entries in the offset stream, not item ordinals. Read both
endpoints in one
+ // ordered pass to avoid revisiting pages for the ends; adjacent rows
share an endpoint:
+ // rowids [1, 2, 8] need offset entries [1, 2, 3, 8, 9].
+ DorisVector<rowid_t> offset_rowids;
+ offset_rowids.reserve(count * 2);
for (size_t i = 0; i < count; ++i) {
- // TODO(cambyszju): now read array one by one, need optimize later
- RETURN_IF_ERROR(seek_to_ordinal(rowids[i]));
+ offset_rowids.push_back(rowids[i]);
+ const auto next_rowid = static_cast<uint64_t>(rowids[i]) + 1;
+ if (next_rowid < _array_reader->num_rows() &&
+ (i + 1 == count || next_rowid != rowids[i + 1])) {
+ offset_rowids.push_back(static_cast<rowid_t>(next_rowid));
+ }
+ }
+ MutableColumnPtr source_offsets_column = ColumnOffset64::create();
+ source_offsets_column->reserve(offset_rowids.size() + 1);
Review Comment:
**Performance validation**
Could we add a focused benchmark comparing the original per-row
implementation, a simpler implementation that batches consecutive parent row
IDs, and this implementation? The new path reduces repeated positioning, but
also introduces O(N) temporary row IDs and offsets plus their allocation cost.
Please include single-row/small batches, highly sparse selections, and
workloads without empty arrays, alongside the contiguous and empty-array cases.
These would help establish both the benefit of batching metadata and the
additional benefit of coalescing item spans across unselected empty arrays.
The new tests verify fewer iterator calls, but do not establish the
latency/CPU or memory trade-off. Measuring these against the same baseline
would help justify the added complexity and check for regressions in cases with
little opportunity to merge ranges.
##########
be/test/storage/segment/column_reader_test.cpp:
##########
@@ -450,6 +474,204 @@ TEST_F(ColumnReaderTest,
NullMapOnlyReadBySparseRowidsAcrossPages) {
EXPECT_EQ(2, nullable_col.get_nested_column().size());
}
+TEST_F(ColumnReaderTest, ArrayReadByRowidsMatchesSequentialReadAcrossPages) {
Review Comment:
**Additional coverage for nested arrays and schema evolution**
The real-file test covers nullable ARRAY<nullable INT>, including the
predicate/filter/lazy sequence. Could we extend coverage to ARRAY<ARRAY<...>>
and ARRAY<STRUCT<...>>, and add a case that reads an old non-nullable ARRAY
segment into a nullable destination after schema evolution?
For the nested cases, please exercise predicate materialization, row
filtering, and lazy filling of the remaining children, and compare the final
result with a full sequential read. This should verify that
already-materialized offsets/null maps remain unchanged and that child data
stays aligned across multiple selected ranges. A SQL regression for nested lazy
materialization would also verify that planner access paths reach the intended
reader phases.
--
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]
