Skip to content

GH-35692: [C++][Parquet] Support to read fixed size list array with nulls #50271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wgtmac merged 5 commits into from
Jul 28, 2026
+105 −23
Merged

GH-35692: [C++][Parquet] Support to read fixed size list array with nulls #50271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8338513
support to read fixed size list array with nulls
HuaHuaY Jun 27, 2026
fb4a9f7
address review
HuaHuaY Jul 9, 2026
bbecb76
address review
HuaHuaY Jul 21, 2026
f8cfd46
address review
HuaHuaY Jul 22, 2026
6b0bd58
address review
HuaHuaY Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 44 additions & 12 deletions cpp/src/parquet/arrow/arrow_reader_writer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3395,23 +3395,55 @@ TEST(ArrowReadWrite, EmptyListView) {
ASSERT_EQ(0, list_view.value_sizes()->size());
}

TEST(ArrowReadWrite, FixedSizeList) {
using ::arrow::field;
using ::arrow::fixed_size_list;
using ::arrow::struct_;

auto type = fixed_size_list(::arrow::int16(), /*size=*/3);
struct FixedSizeListTestCase {
std::shared_ptr<DataType> type;
std::string json;
};

const char* json = R"([
[1, 2, 3],
[4, 5, 6],
[7, 8, 9]])";
auto array = ::arrow::ArrayFromJSON(type, json);
auto table = ::arrow::Table::Make(::arrow::schema({field("root", type)}), {array});
class TestFixedSizeListRoundTrip
: public ::testing::TestWithParam<FixedSizeListTestCase> {};

static const std::vector<FixedSizeListTestCase> kFixedSizeListTestCases = {
{.type = ::arrow::fixed_size_list(::arrow::int16(), /*list_size=*/3), .json = R"([
{"root": [1, 2, 3]},
{"root": [4, 5, 6]},
{"root": [7, 8, 9]}])"},
{.type = ::arrow::fixed_size_list(::arrow::int16(), /*list_size=*/3), .json = R"([
{"root": null},
{"root": [1, 2, 3]},
{"root": null},
{"root": [4, 5, 6]},
{"root": null}])"},
{.type = ::arrow::fixed_size_list(::arrow::int16(), /*list_size=*/3), .json = R"([
{"root": null},
{"root": null},
{"root": null}])"},
{.type = ::arrow::fixed_size_list(
::arrow::fixed_size_list(::arrow::int16(), /*list_size=*/2),
/*list_size=*/2),
.json = R"([
{"root": [[1, 2], [3, 4]]},
{"root": null},
{"root": [[5, 6], null]},
{"root": [null, [7, 8]]}])"},
{.type = ::arrow::list(::arrow::fixed_size_list(::arrow::int16(), /*list_size=*/2)),
.json = R"([
{"root": [[1, 2], null, [3, 4]]},
{"root": null},
{"root": [null, [5, 6]]},
{"root": []}])"}};

TEST_P(TestFixedSizeListRoundTrip, RoundTrip) {
const auto& test_case = GetParam();
auto table = ::arrow::TableFromJSON(
::arrow::schema({::arrow::field("root", test_case.type)}), {test_case.json});
auto props_store_schema = ArrowWriterProperties::Builder().store_schema()->build();
CheckSimpleRoundtrip(table, 2, props_store_schema);
}

INSTANTIATE_TEST_SUITE_P(ArrowReadWrite, TestFixedSizeListRoundTrip,
::testing::ValuesIn(kFixedSizeListTestCases));

TEST(ArrowReadWrite, ListOfStructOfList2) {
using ::arrow::field;
using ::arrow::list;
Expand Down
72 changes: 61 additions & 11 deletions cpp/src/parquet/arrow/reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

#include <algorithm>
#include <cstring>
#include <iterator>
#include <memory>
#include <random>
#include <span>
#include <unordered_set>
#include <utility>
#include <vector>

#include "arrow/array.h"
#include "arrow/array.h" // IWYU pragma: keep
#include "arrow/array/concatenate.h"
#include "arrow/buffer.h"
#include "arrow/extension_type.h"
#include "arrow/io/memory.h"
Expand All @@ -35,6 +37,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/async_generator.h"
#include "arrow/util/bit_run_reader.h"
#include "arrow/util/bit_util.h"
#include "arrow/util/future.h"
#include "arrow/util/iterator.h"
Expand All @@ -45,13 +48,10 @@
#include "arrow/util/type_traits.h"

#include "parquet/arrow/reader_internal.h"
#include "parquet/bloom_filter.h"
#include "parquet/bloom_filter_reader.h"
#include "parquet/column_reader.h"
#include "parquet/exception.h"
#include "parquet/file_reader.h"
#include "parquet/metadata.h"
#include "parquet/page_index.h"
#include "parquet/properties.h"
#include "parquet/schema.h"

Expand Down Expand Up @@ -725,13 +725,63 @@ 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 has_elements) -> Status {
const int32_t expected_size = has_elements ? 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(

@pitrou pitrou Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure C++20 ranges are available on all our CI platforms unfortunately (some of them require old compilers).

@HuaHuaY HuaHuaY Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are no compilation errors in the CI pipelines which are already run.

@pitrou pitrou Jul 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me enable the more annoying ones :)

@HuaHuaY HuaHuaY Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new CI tests have also been compiled, so I haven't changed the code yet.

@pitrou pitrou Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thisisnic Do you remember how to launch the R builds with the most outdated compilers? :)

@thisisnic thisisnic Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not off the top of my head, but there should be a recent-ish PR which fixed the last ones that broke it, and will have the CI job being run on it in the comments.

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 (has_elements) {
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 has_elements) -> Status {
RETURN_NOT_OK(validate_offsets(start, length, has_elements));

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 (!has_elements) {
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)));

@wgtmac wgtmac Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid creating one Array per validity run? This could become expensive for wide fixed-size lists. Is it possible to use a single builder using AppendArraySlice() and AppendNulls() instead?

@HuaHuaY HuaHuaY Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea. First, the array is created via Slice, and the actual data isn't copied. Second, Concatenate handles different array types automatically, whereas doing it manually would be too much of a hassle.

@wgtmac wgtmac Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@HuaHuaY HuaHuaY Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a TODO comment in the latest commit.

return Status::OK();
};

DCHECK_NE(data->buffers[0], nullptr);
RETURN_NOT_OK(::arrow::internal::VisitBitRuns(
data->buffers[0]->data(), data->offset, data->length, visit_run));

// TODO(GH-50271): Build one padded child array directly instead of creating
// one temporary Array/ArrayData per validity run and concatenating them.
ARROW_ASSIGN_OR_RAISE(auto child_array_with_padding,
::arrow::Concatenate(child_arrays, ctx_->pool));
data->child_data[0] = child_array_with_padding->data();
} else {
RETURN_NOT_OK(validate_offsets(/*start=*/0, data->length, /*valid=*/true));
}
data->buffers.resize(1);
std::shared_ptr<Array> result = ::arrow::MakeArray(data);
Expand Down
Loading