PARQUET-2013: [Format] Mention that ConvertedType is deprecated#169
PARQUET-2013: [Format] Mention that ConvertedType is deprecated#169pitrou merged 1 commit intoapache:masterfrom
Conversation
Also slight wording improvements, and replace a "must" with "should" for writing the ConvertedType field.
gszadovszky
left a comment
gszadovszky
left a comment
There was a problem hiding this comment.
Thanks a lot for taking care of this. Let's wait for another 24 hours for any other comments.
| * | ||
| * To maintain compatibility, implementations using LogicalType for a | ||
| * SchemaElement must also set the corresponding ConvertedType from the | ||
| * SchemaElement should also set the corresponding ConvertedType from the |
There was a problem hiding this comment.
sorry for the belated comment by why the change from must to should?
There was a problem hiding this comment.
I also believed that setting ConvertedType was MUST for forward compatibility reasons. Can we please discuss on the mailing list?
There was a problem hiding this comment.
Logical types were introduced 3.5 years go, so it seemed reasonable to relax the requirement. Do you expect some Parquet readers in the wild to still not understand them?
It's easy, for every converted type you support, to also support the corresponding logical type. It's only a small amount of logic in the decoding path, and it's not performance-critical.
There was a problem hiding this comment.
That said, feel free to spawn a discussion on the ML.
There was a problem hiding this comment.
If you want to make this change, I think it needs to be voted on. Knowing the motley state of Parquet implementations in enterprise deployments I would be uncomfortable writing files that don't have the field.
There was a problem hiding this comment.
Well, given the state, even today, of inter-implementation compatibility, I think anyone using a 2017-era implementation would run into lots of issues when reading files produced by other implementations, regardless of whether logical types are used.
There was a problem hiding this comment.
That said, if people feel strongly about it, I can simply revert this one-word change, because I don't think there's any point organizing a vote on it.
There was a problem hiding this comment.
It's a change with forward compatibility implications, so either we revert or we discuss/vote, but we can't leave it like this. Impala for example only started to get support for LogicalType in 2019 (and this is only one of the mainstream independent implementations), so we'd be making a bet about how quickly updates are rolling out to all of the corners of the world
4 participants
Also slight wording improvements, and replace a "must" with "should" for writing the ConvertedType field.