github-actions[bot] commented on code in PR #67817:
URL: https://github.com/apache/doris/pull/67817#discussion_r4063231357
##########
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);
+ RETURN_IF_ERROR(_offset_iterator->read_by_rowids(offset_rowids.data(),
offset_rowids.size(),
+ source_offsets_column));
+ // source_offsets contains element ordinals in the segment, not file byte
positions.
+ // Destination offsets instead delimit the compact item stream after row
selection.
+ auto& source_offsets =
assert_cast<ColumnOffset64&>(*source_offsets_column).get_data();
+ DORIS_CHECK(source_offsets.size() == offset_rowids.size());
+ if (static_cast<uint64_t>(rowids[count - 1]) + 1 ==
_array_reader->num_rows()) {
Review Comment:
[P1] Preserve arbitrary row-ID order when resolving the final ARRAY row
`read_by_rowids` is also called by `FixedReadPlan`, which preserves incoming
order; `HistoricalRowFetcherTest.ReadColumnsReturnsThePlannedRows` explicitly
reads rows `[2, 0]`. For a three-row ARRAY, this path builds endpoint IDs `[2,
0, 1]`, but this check only appends the tail sentinel when the final physical
row is also the last requested element. It therefore pairs `offset(2)` with
`offset(0)` and returns `Corruption`; if those offsets are equal, the next
iteration indexes past `source_offsets`. Partial updates support missing ARRAY
columns, so this breaks a valid non-row-store update path. Please preserve
arbitrary requested order—for example, resolve an end per input row, or take
the former per-row fallback before mutating metadata—and add ARRAY coverage
with the final physical row before an earlier row.
--
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]
