[GitHub] [arrow-rs] alamb commented on a change in pull request #258: support full u64 roundtrip through parquet

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #258:
URL: https://github.com/apache/arrow-rs/pull/258#discussion_r626663562



##########
File path: parquet/src/arrow/array_reader.rs
##########
@@ -380,6 +380,18 @@ impl<T: DataType> ArrayReader for PrimitiveArrayReader<T> {
                 }
                 Arc::new(builder.finish()) as ArrayRef
             }
+            ArrowType::UInt64 => match array.data_type() {

Review comment:
       This code was confusing to me (as it is manipulating the Arrow array after it has been created rather than manipulating the data prior to creating the array). 
   
   It also seems like it will result in a copy of all Int64 columns which may not be idea.
   
   I wonder if you considered creating the `Unit64Array` directly from the parquet data up here: https://github.com/apache/arrow-rs/pull/258/files#diff-0d6bed48d78c5a2472b7680a8185cabdc0bd259d6484e184439ed7830060661fR316-R319
    instead?
   
   That may be clearer to understand as well as more performant
   




-- 
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:
users@infra.apache.org