ARROW-7325: [Rust] [Parquet] Update to parquet-format 2.6 and thrift 0.12 #5971
Conversation
| parquet::Type::DOUBLE => Type::DOUBLE, | ||
| parquet::Type::BYTE_ARRAY => Type::BYTE_ARRAY, | ||
| parquet::Type::FIXED_LEN_BYTE_ARRAY => Type::FIXED_LEN_BYTE_ARRAY, | ||
| parquet::Type::Boolean => Type::BOOLEAN, |
There was a problem hiding this comment.
@kornholi @liurenjie1024 @sunchao do you think it would be disruptive to change the local enums like Type and LogicalType to have camel casing? I'm assuming we've had them as upper case because earlier versions of thrift generated them like that?
There was a problem hiding this comment.
Arrow's enum has camel casing, so I think it may look better to change parquet's local enums to have camel casing.
There was a problem hiding this comment.
I'm somewhat confused why these duplicate enums exist in the first place. If we're going to depend on a specific version of parquet-format, why not use the types from there directly?
There was a problem hiding this comment.
Good question. I'll have a look to see why, but my answer won't be authoritative as @sunchao, @sadikovi and @liurenjie1024 are most familiar with the parquet codebase.
There was a problem hiding this comment.
I'm not familiar with this part, but I guess this is used for better mapping of definitions of parquet.
There was a problem hiding this comment.
I think originally this was to give an isolation between the Thrift definitions and the definitions in parquet-rs, and also allow us to rename things a little differently.
There was a problem hiding this comment.
Yes the Thrift was using all upper cases before and changed because of the version bump. We may break dependencies if we change the cases for the local enums. Need to be careful with that. I'd say do that in a separate PR later.
kornholi
force-pushed
the
update-pqf-and-thrift
branch
from
b4884d1
to
1410bf6
Compare
Primary motivation is to pull in newer versions of
byteorder,num-traits, andordered-float.PTAL @sunchao @nevi-me