Uh oh!
There was an error while loading. Please reload this page.
WIP Upgrade to arrow-rs/parquet 54.0.0 - #13663
Conversation
bd6517c to
44bb39dCompareUh oh!
There was an error while loading. Please reload this page.
54.0.0Uh oh!
There was an error while loading. Please reload this page.
3c5f100 to
51fdc1eCompare| } | ||
| // TODO file a ticket about handling deprecated dict_id attributes | ||
| #[allow(deprecated)] |
There was a problem hiding this comment.
I don't know what to do here:
- Should we remove the
field_idfrom DataFusion serialization code (as it is planned for removal eventually in DataFusion)? - Should we un-deprecate
field_idin arrow-rs? - Something else?
@avantgardnerio, @thinkharderdev and @Dandandan , this test was added in
However, @brancz's upstream PR in arrow-rs has deprecated dict_id on Field and all related APIs with the idea it will be eventually removed (and become a detail of the IPC transport rather than visible in the Schema)
- Add deprecation warnings for everything related to
dict_idarrow-rs#6873 upstream in arrow-rs
@thinkharderdev mentioned apache/arrow-rs#6873 (comment)
If people are using the dictionary ID it's probably for something that should properly be done with kv metadata instead.
So one possibility could be to provide some more documentation / writeup as an upgrade guide for arrow explaining how to use field metadata instead ?
Please advise
There was a problem hiding this comment.
If people are using the dictionary ID it's probably for something that should properly be done with kv metadata instead
I mostly meant that if you give people 32 bits of space someone, somewhere will use it as metadata if they don't need it for anything else. I don't really have any particular use case in mind.
I don't have any objection to removing the field from datafusion serde. When we added that test we were still fighting to try and ensure unique dict IDs but we've long since moved on and found better ways around it.
There was a problem hiding this comment.
Or maybe on second thought we should keep it around until it is fully removed from upstream?
There was a problem hiding this comment.
Yeah, that sounds like a reasonable approach. I'll update the comments to match if no one else has a usecase
There was a problem hiding this comment.
Agreed. Also, #8457 (comment) suggests it was never important to set the dict id directly, just that it wasn't sent correctly on the wire and already called out what to do once we remove the dict id. There were in fact a number of dictfixes that were necessary for us to correctly communicate the dicts correctly over the wire as well.
51fdc1e to
fb080a2Comparefb080a2 to
2fe785bCompare| /// (writing) Sets max statistics size for any column. If NULL, uses | ||
| /// default parquet writer setting | ||
| pub max_statistics_size: Option<usize>, default = Some(4096) |
There was a problem hiding this comment.
I think simply removing the config option is pretty rough as you end up with errors liek this
External error: query failed: DataFusion error: Invalid or Unsupported Configuration: Config value "max_statistics_size" not found on ParquetOptions
The best thing to do would be to mark the config setting deprecated, but there wasn't an obvious way to do this with the existing macros. I will futz with the macros and see if I can make it deprecated somehow
| fn try_from(field: &protobuf::Field) -> Result<Self, Self::Error> { | ||
| let datatype = field.arrow_type.as_deref().required("arrow_type")?; | ||
| let field = if field.dict_id != 0 { | ||
| // TODO file a ticket about handling deprecated dict_id attributes |
There was a problem hiding this comment.
Need to update per apache/arrow-rs#6885 I think
I would appreciate this PR getting in before the next datafusion release 🙏 |
| /// Sets max statistics size for the column path. If NULL, uses | ||
| /// default parquet options | ||
| pub max_statistics_size: Option<usize>, default = None |
There was a problem hiding this comment.
I think it is a mistake to remove this field directly -- we should put it back in and figure out some way to mark it deprecated
alamb
commented
Jan 12, 2025
Yes I hope to -- if anyone has time to help get it in shape / make a new PR it would be most appreciated I think a good first step would be to upgrade DataFusion to use the new arrow / parquet versions and sprinkle some Then we can file follow on tickets to sort out exactly how to deal with the deprecated APIs as a follow on PR I'll try to work on this over the next week if no one beats me to it |
alamb
commented
Jan 13, 2025
I added this to the list of items that i think we should fix before releasing version 45.0.0: |
Owen-CH-Leung
commented
Jan 14, 2025
Do you still need help on this ? I can help to file a MR tmr to upgrade arrow, parquet & pyo3 for the project. Let me know |
alamb
commented
Jan 15, 2025
@Owen-CH-Leung -- Yes, please I am still looking for help. Feel free to create a PR and use any/all fo the code from this one as necessary. Thank you 🙏 I will post on this PR when I begin to work on it if you haven't beaten me to it |
Which issue does this PR close?
Related to
54.0.0(December 2024) arrow-rs#6342Rationale for this change
Prepare DataFusion for the next arrow-rs release
Also, help test to ensure that the next arrow-rs release has no regressions by testing with DataFusion before we release it
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?