GH-465: Clarify backward-compatibility rules on LIST type #466
+17
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wgtmac
force-pushed
the
old-list-structure
branch
3 times, most recently
from
abc3d1d
to
d9579bc
Compare
|
BTW, can you also point out the Java code in this pr? |
mapleFU
reviewed
Oct 30, 2024
|
Other LGTM but I think it worths issue a disscussion... |
wgtmac
force-pushed
the
old-list-structure
branch
2 times, most recently
from
4551c79
to
f200d34
Compare
|
I have sent a discussion thread to the dev ML. It would be good if you can take a look. Thanks! @emkornfield @pitrou @gszadovszky @rdblue @etseidl @clairemcginty |
wgtmac
commented
Oct 30, 2024
wgtmac
force-pushed
the
old-list-structure
branch
from
f200d34
to
fc8aca3
Compare
etseidl
approved these changes
Oct 30, 2024
| repeated type is the element type and elements are required. | ||
| element type and elements are required. In this case, the element type is | ||
| a Struct type with multiple fields. | ||
| 3. If the repeated field is a group (without annotation) with one `required` or |
There was a problem hiding this comment.
Suggested change
| 3. If the repeated field is a group (without annotation) with one `required` or | |
| 3. If the repeated field is a group (without `LIST` annotation) with one `required` or |
Not sure if this should be made explicit.
emkornfield
reviewed
Oct 30, 2024
| repeated type is the element type and elements are required. | ||
| element type and elements are required. In this case, the element type is | ||
| a Struct type with multiple fields. | ||
| 3. If the repeated field is a group (without annotation) with one `required` or |
There was a problem hiding this comment.
I might leave the entire block as it was previously and clarify that:
- Without annotatation the type is a single struct/tuple with the element in it.
- With a logical type annotation (I think only LIST and MAP apply here).
I also think we need to update the documentation above:
The outer-most level must be a group annotated with `LIST` that contains a
single field named `list`. The repetition of this level must be either
`optional` or `required` and determines whether the list is nullable.
To point to the fact that for backwards compatibility it can also be repeated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The C++ reader of parquet-cpp is having a hard time to read Parquet file written by parquet-java with
parquet.avro.write-old-list-structure=trueand schema below:See apache/arrow#43994 and apache/arrow#43995
What changes are included in this PR?
Clarify the rules and add an example.
Do these changes have PoC implementations?
Not required
Closes #465