Allow reading more than 32k Parquet row groups#10149
Conversation
etseidl
added
api-change
|
If this is needed sooner, I can revert the public changes and add an i32 accessor for use by the row numbering. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks good to me @etseidl -- thank you
I think we could potentially reduce the change required in this PR (and not have to change any APIs) by simply not populating the ordinal field for row groups with indexes that are too large and keep returning i16
What do you think?
| let ordinal = self.row_group_index; | ||
|
|
||
| let ordinal: i16 = ordinal.try_into().map_err(|_| { | ||
| // Thrift cannot encode lists with more than 2B elements |
There was a problem hiding this comment.
I am not sure what the 2B elements refers to
| self.write_path_in_schema | ||
| } | ||
|
|
||
| /// Control the writing of the `ordinal` element of the `RowGroup` struct. |
There was a problem hiding this comment.
Should we add some context here about why one would disable writing the optional ordinal field? Namely because it can't be set for row groups greater than i32::MAX?
| let mut writer = SerializedFileWriter::new(&file, schema, props).unwrap(); | ||
|
|
||
| // Create 32k + 1 empty rowgroups. No row group ordinals should be written (but we can't | ||
| // test for that). |
There was a problem hiding this comment.
and arguably it is an implementation detail -- just testing writing 32k row groups is enough
| #[allow(unused_assignments)] | ||
| fn write_thrift<W: Write>(&self, writer: &mut ThriftCompactOutputProtocol<W>) -> Result<()> { | ||
| writer.set_write_path_in_schema(self.write_path_in_schema); | ||
| // only write ordinal if all values will fit in an i16 |
There was a problem hiding this comment.
Would it be possible to write ordinal for the first 2^16-1 row groups, and then just not write it for larger row groups. I think you could avoid changing the thrift writer / adding set_write_row_group_ordinal
2 participants
Which issue does this PR close?
RowGroup.ordinalis absent #10129.Rationale for this change
The parquet crate will error if more than 32767 row groups are present in the file. This is a limit imposed on write when encryption is in use, but there is no other limit on the number of row groups beyond that imposed by the Thrift compact protocol.
What changes are included in this PR?
This changes the
ordinalfield of theRowGroupMetaDatafrom ani16toi32. This allows reading up to the maximum number of row groups allowed by Thrift. On write, theordinalon the RowGroup will not be written if more than 32k row groups are present.Are these changes tested?
Yes
Are there any user-facing changes?
Yes,
RowGroupMetaData::ordinalnow returnsOption<i32>andRowGroupMetaDataBuilder::set_ordinaltakesOption<i32>.