sunchao commented on a change in pull request #9592:
URL: https://github.com/apache/arrow/pull/9592#discussion_r584495886
##########
File path: rust/parquet/src/schema/types.rs
##########
@@ -972,18 +1011,22 @@ fn from_thrift_helper(
}
/// Method to convert to Thrift.
-pub fn to_thrift(schema: &Type) -> Result<Vec<SchemaElement>> {
+pub fn to_thrift(schema: &Type, writer_version: i32) ->
Result<Vec<SchemaElement>> {
Review comment:
Hmm, the `LogicalType` is not version 2 only though: it was introduced
in 2.4.0. It is also an optional field which means backward compatibility.
Therefore, I guess it should be fine to write it no matter the `writer_version`
is 1 or 2? A reader > 2.4.0 will try to parse the optional `LogicalType` first
while one < 2.4.0 will just look at the `ConvertedType` field.
I don't fully understand the implication of parsing schema. When looking at
`INTEGER(32,true)`, shouldn't we just populate the `LogicalType` together with
the `ConvertedType` field? and only populate `ConvertedType` when seeing
`INT_32`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
