GH-35692: [C++][Parquet] Support to read fixed size list array with nulls - #50271
Conversation
| int32_t expected_size) -> Status { | ||
| std::span<const int32_t> run_offsets(offsets + start, | ||
| static_cast<size_t>(length + 1)); | ||
| const auto first_invalid_offset = std::ranges::adjacent_find( |
There was a problem hiding this comment.
I'm not sure C++20 ranges are available on all our CI platforms unfortunately (some of them require old compilers).
There was a problem hiding this comment.
It looks like there are no compilation errors in the CI pipelines which are already run.
There was a problem hiding this comment.
Let me enable the more annoying ones :)
There was a problem hiding this comment.
The new CI tests have also been compiled, so I haven't changed the code yet.
There was a problem hiding this comment.
@thisisnic Do you remember how to launch the R builds with the most outdated compilers? :)
There was a problem hiding this comment.
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.
|
@rok FYI :) |
915c757 to
2fc0d49
Compare
| return Status::OK(); | ||
| } | ||
| child_arrays.push_back( | ||
| ::arrow::MakeArray(data->child_data[0]->Slice(offsets[start], child_length))); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
There was a problem hiding this comment.
I have added a TODO comment in the latest commit.
|
Codex reminded me that code below is outdated after this fix and it seems that arrow/cpp/src/parquet/level_conversion.cc Lines 122 to 126 in 0077539 |
2fc0d49 to
c5cadcf
Compare
This requires modifying the public methods in |
|
@HuaHuaY Are you waiting for another review here? |
If there are no new comments, I think this PR is ok to be merged. |
c5cadcf to
3840ffc
Compare
|
I'll merge this tomorrow if no objection. |
|
@github-actions crossbow submit -g r |
I've triggered the R builds just to make sure we don't have potential failures suggested in #50271 (comment) |
It seems that no extra ci starts. I might be because @pitrou has added "CI: Extra R". R's CI has already been run. |
|
Revision: 06266f9 Submitted crossbow builds: ursacomputing/crossbow @ actions-d711fbc610 |
|
Oh. Extra CI starts. It just takes a little while to start up. |
|
The additional CI run revealed an issue. Arrow recently introduced |
|
I think these failures are unrelated to this PR. To be sure, I'll rebase and run it again. |
06266f9 to
6b0bd58
Compare
|
@github-actions crossbow submit -g r |
|
|
@github-actions crossbow submit -g r |
|
Revision: 6b0bd58 Submitted crossbow builds: ursacomputing/crossbow @ actions-523da37569 |
|
I've checked that all CI failures are unrelated. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5e698fe. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Support to read fixed size list array with nulls.
What changes are included in this PR?
Modified
FixedSizeListReaderto add null slots toFixedSizeList.Are these changes tested?
Yes.
Are there any user-facing changes?
User can read fixed size list array with nulls now.
This PR also fixes GH-35697.