Repository navigation
Conversation
Reading several row groups at once can make a leaf reader return one chunk per row group, for example a dictionary leaf where each row group has its own dictionary. List and struct readers rejected this with NotImplemented. Concatenate the chunks instead, and propagate the error from Concatenate when the result does not fit in the type, for example binary offsets beyond 2GiB.
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
github-actions
Bot
added
awaiting committer review
| ARROW_SCOPED_TRACE("type = ", type->ToString()); | ||
| auto array = | ||
| ::arrow::ArrayFromJSON(type, R"([["a"], ["b", "a"], ["c", "a"], ["b"]])"); | ||
| auto table = ::arrow::Table::Make(::arrow::schema({field("root", type)}), {array}); |
There was a problem hiding this comment.
This test generates two row groups and both retain the full input dictionary ["a", "b", "c"], so this exercises multiple chunks with identical dictionaries.
Could we add a separate case with two independently built input chunks aligned with the row groups, and compare the full-table read with a separately built unified expected table?
There was a problem hiding this comment.
Thanks, I want to make sure I understand. I’d add a separate test that:
- builds an input ChunkedArray of list<dictionary> with two chunks, each with its own dictionary ([a, b] and [c, a, b]);
- writes it with a row group size equal to the chunk length, so each chunk becomes one row group;
- reads it back with ReadTable();
- compares against an expected table built separately, from the same rows, with a single dictionary.
My concern is that comparing dictionary-typed arrays also compares the dictionary order, so the test would then depend on how Concatenate orders the merged dictionary. That is what the fix delegates to, but it isn’t what it is meant to guarantee. Would it be fine to cast the result to list and compare the decoded values against an expected list, and keep a schema check so the result is still the dictionary type?
Rationale for this change
Adresses part of issue #32723.
Using the parquet ReadTable functions fails on list of dicts, if they contain more than one row group.
In such case, the data is in a chunked array. A NotImplemented guards stops this from being read.
What changes are included in this PR?
This PR is a minimal fix that addresses this in the case when the chunks can be combined into a single array.
It does not address the overflow issues, and the preexisting arrow::Concatenate will throw on that. The error is propagated.
Are these changes tested?
One test case is added for this specific situation. It fails without the fix in reader.cc.
No regressions on
parquet-arrow-reader-writer-testfound, which I also tested.On macOS.
Are there any user-facing changes?
The users will be able to read a few more parquet files that were previously rejected.
Was AI used for this PR?
Yes.
Claude (Opus and Sonnet 5.5) was used for compiling information from existing issues and propose a solution. I decided to do a minimal fix, not addressing the overflow problems, to make sure I understand the PR. I also asked it to verify style guide and contributor process on https://arrow.apache.org/docs/dev/developers/guide/step_by_step/ hoping I dont miss any steps.
PR code and description written by:
Reviewed before submission by: