wgtmac commented on code in PR #50271:
URL: https://github.com/apache/arrow/pull/50271#discussion_r3631678184
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -725,13 +725,59 @@ class PARQUET_NO_EXPORT FixedSizeListReader : public
ListReader<int32_t> {
DCHECK_EQ(data->buffers.size(), 2);
DCHECK_EQ(field()->type()->id(), ::arrow::Type::FIXED_SIZE_LIST);
const auto& type =
checked_cast<::arrow::FixedSizeListType&>(*field()->type());
- const int32_t* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
- for (int x = 1; x <= data->length; x++) {
- int32_t size = offsets[x] - offsets[x - 1];
- if (size != type.list_size()) {
- return Status::Invalid("Expected all lists to be of size=",
type.list_size(),
- " but index ", x, " had size=", size);
+ const auto* offsets = reinterpret_cast<const
int32_t*>(data->buffers[1]->data());
+ const int32_t list_size = type.list_size();
+ auto validate_offsets = [&](int64_t start, int64_t length, bool valid) ->
Status {
+ const int32_t expected_size = valid ? list_size : 0;
+ std::span<const int32_t> run_offsets(offsets + start,
+ static_cast<size_t>(length + 1));
+ const auto first_invalid_offset = std::ranges::adjacent_find(
+ run_offsets,
+ [&](int32_t left, int32_t right) { return right - left !=
expected_size; });
+ if (first_invalid_offset != run_offsets.end()) {
+ const int64_t x =
+ start + std::ranges::distance(run_offsets.begin(),
first_invalid_offset);
+ const int32_t size = offsets[x + 1] - offsets[x];
+ if (valid) {
+ return Status::Invalid("Expected all lists to be of size=",
list_size,
+ " but index ", x + 1, " had size=", size);
+ }
+ return Status::Invalid("Expected null fixed-size list at index ", x +
1,
+ " to have no child values but had size=", size);
}
+ return Status::OK();
+ };
+ if (data->GetNullCount() != 0) {
+ // Rebuild the child array run-by-run so null fixed-size list slots still
+ // contribute list_size child values in the final layout.
+ ::arrow::ArrayVector child_arrays;
+
+ auto visit_run = [&](int64_t start, int64_t length, bool valid) ->
Status {
+ RETURN_NOT_OK(validate_offsets(start, length, valid));
+
+ const int64_t child_length = length * list_size;
+ // Valid runs reuse the decoded child slice; null runs materialize null
+ // children to preserve the fixed-size list shape.
+ if (!valid) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto null_array,
+ ::arrow::MakeArrayOfNull(type.value_type(), child_length,
ctx_->pool));
+ child_arrays.push_back(std::move(null_array));
+ return Status::OK();
+ }
+ child_arrays.push_back(
+ ::arrow::MakeArray(data->child_data[0]->Slice(offsets[start],
child_length)));
Review Comment:
My concern is the per-run object/allocation overhead rather than copying
each valid slice: an alternating validity bitmap still creates O(number of
runs) Array/ArrayData objects and O(number of null runs) null arrays before the
final concatenation.
But I agree that my suggestion requires a lot of changes to handle different
child types. I'm fine to keep it as-is by adding a TODO comment for a future
improvement?
--
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]
