adonis0147 commented on code in PR #12293: URL: https://github.com/apache/doris/pull/12293#discussion_r969306888
########## be/src/olap/rowset/segment_v2/column_reader.cpp: ########## @@ -466,6 +486,42 @@ Status ArrayFileColumnIterator::next_batch(size_t* n, ColumnBlockView* dst, bool return Status::OK(); } +Status ArrayFileColumnIterator::_seek_by_offsets(ordinal_t ord) { + // using offsets info + ordinal_t offset = 0; + RETURN_IF_ERROR(_peek_one_offset(&offset)); + RETURN_IF_ERROR(_item_iterator->seek_to_ordinal(offset)); + return Status::OK(); +} + +Status ArrayFileColumnIterator::seek_to_ordinal(ordinal_t ord) { + RETURN_IF_ERROR(_offset_iterator->seek_to_ordinal(ord)); + if (_array_reader->is_nullable()) { + RETURN_IF_ERROR(_null_iterator->seek_to_ordinal(ord)); + } + return _seek_by_offsets(ord); +} + +Status ArrayFileColumnIterator::_caculate_offsets(ssize_t start, + vectorized::MutableColumnPtr& offsets, + size_t* num_items) { Review Comment: ```suggestion Status ArrayFileColumnIterator::_calculate_offsets(vectorized::ColumnArray::ColumnOffsets& offsets, ssize_t start) ``` 1. Typo: `_caculate_offsets` -> `_calculate_offsets` 2. For function arguments, I think it is better to put the mutable arguments before the immutable ones. ########## be/src/olap/rowset/segment_v2/column_reader.cpp: ########## @@ -466,6 +486,42 @@ Status ArrayFileColumnIterator::next_batch(size_t* n, ColumnBlockView* dst, bool return Status::OK(); } +Status ArrayFileColumnIterator::_seek_by_offsets(ordinal_t ord) { + // using offsets info + ordinal_t offset = 0; + RETURN_IF_ERROR(_peek_one_offset(&offset)); + RETURN_IF_ERROR(_item_iterator->seek_to_ordinal(offset)); + return Status::OK(); +} + +Status ArrayFileColumnIterator::seek_to_ordinal(ordinal_t ord) { + RETURN_IF_ERROR(_offset_iterator->seek_to_ordinal(ord)); + if (_array_reader->is_nullable()) { + RETURN_IF_ERROR(_null_iterator->seek_to_ordinal(ord)); + } + return _seek_by_offsets(ord); +} + +Status ArrayFileColumnIterator::_caculate_offsets(ssize_t start, + vectorized::MutableColumnPtr& offsets, + size_t* num_items) { + auto& column_offsets = static_cast<vectorized::ColumnArray::ColumnOffsets&>(*offsets); + ordinal_t last_offset = 0; + RETURN_IF_ERROR(_peek_one_offset(&last_offset)); + + auto& offsets_data = column_offsets.get_data(); + *num_items = last_offset - offsets_data[start]; Review Comment: We would better do single job in one function. According to the name of this function, we should calculate the new offsets only and put the assignment of `num_items` **outside** the function. -- 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: commits-unsubscr...@doris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org