Copilot commented on code in PR #61991:
URL: https://github.com/apache/doris/pull/61991#discussion_r3019890126
##########
be/src/storage/segment/variant/variant_column_writer_impl.cpp:
##########
@@ -222,35 +224,35 @@ void build_doc_value_stats(const ColumnVariant& variant,
DocValuePathStats* stat
}
}
-// Materialize sparse subcolumns for each path and build per-path non-null
counts.
+// Materialize sparse subcolumns for each path using precomputed per-path
non-null counts.
// For each row, we decode only present (path, value) pairs and append them to
the
// corresponding subcolumn, while recording the row id to allow gap filling
later.
-void build_sparse_subcolumns_and_stats(const ColumnVariant& variant,
- DocSparseSubcolumns* subcolumns,
DocValuePathStats* stats) {
+void build_sparse_subcolumns(const ColumnVariant& variant, const
DocValuePathStats& stats,
+ DocSparseSubcolumns* subcolumns) {
auto [column_key, column_value] =
variant.get_doc_value_data_paths_and_values();
const auto& column_offsets = variant.serialized_doc_value_column_offsets();
const size_t num_rows = column_offsets.size();
subcolumns->clear();
- stats->clear();
- subcolumns->reserve(column_key->size());
+ subcolumns->reserve(stats.size());
for (size_t row = 0; row < num_rows; ++row) {
const size_t start = column_offsets[row - 1];
Review Comment:
`build_sparse_subcolumns()` also computes `start = column_offsets[row - 1]`
with `row` starting at 0, which underflows for the first row. Please use `start
= (row == 0) ? 0 : column_offsets[row - 1]` to avoid undefined behavior and
ensure row 0 is decoded correctly.
```suggestion
const size_t start = (row == 0) ? 0 : column_offsets[row - 1];
```
##########
be/src/exec/common/variant_util.cpp:
##########
@@ -1901,11 +1902,12 @@ phmap::flat_hash_map<std::string_view,
ColumnVariant::Subcolumn> materialize_doc
DCHECK_EQ(num_rows, variant.size()) << "doc snapshot offsets size mismatch
with variant rows";
- // Best-effort reserve: at most number of kv pairs.
- subcolumns.reserve(column_key->size());
+ subcolumns.reserve(expected_unique_paths != 0
+ ? expected_unique_paths
+ : std::min<size_t>(column_key->size(),
kInitialPathReserve));
for (size_t row = 0; row < num_rows; ++row) {
- const size_t start = (row == 0) ? 0 : column_offsets[row - 1];
+ const size_t start = column_offsets[row - 1];
Review Comment:
`materialize_docs_to_subcolumns_map()` computes `start` as
`column_offsets[row - 1]` while iterating `row` from 0, which underflows for
the first row and can read invalid memory. Use `start = (row == 0) ? 0 :
column_offsets[row - 1]` (same pattern as other offset-based loops) to avoid UB
and incorrect decoding for row 0.
```suggestion
const size_t start = (row == 0) ? 0 : column_offsets[row - 1];
```
##########
be/src/storage/segment/variant/binary_column_reader.cpp:
##########
@@ -107,6 +107,16 @@ BinaryColumnType SingleSparseColumnReader::get_type()
const {
}
Status
MultipleBinaryColumnReader::new_binary_column_iterator(ColumnIteratorUPtr*
iter) const {
+ // Single bucket can be read directly without cross-bucket merge/sort.
+ if (_multiple_column_readers.size() == 1) {
+ DCHECK(!_multiple_column_readers.empty());
+ auto it = _multiple_column_readers.begin();
+ ColumnIteratorUPtr single_iter;
+ RETURN_IF_ERROR(it->second->new_iterator(&single_iter, nullptr));
Review Comment:
In the single-bucket fast path, `it->second` is dereferenced without the
null check used in the multi-bucket loop. If a null `ColumnReader` is ever
inserted, this will crash; mirror the `if (!reader) return
Status::NotFound(...)` check before calling `new_iterator()`.
```suggestion
auto index = it->first;
auto reader = it->second;
if (!reader) {
return Status::NotFound("No column reader available, binary
column index is: ", index);
}
ColumnIteratorUPtr single_iter;
RETURN_IF_ERROR(reader->new_iterator(&single_iter, nullptr));
```
--
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]
