alamb commented on code in PR #8048:
URL: https://github.com/apache/arrow-rs/pull/8048#discussion_r2258266438


##########
parquet/src/basic.rs:
##########
@@ -817,6 +877,90 @@ impl From<ConvertedType> for 
Option<parquet::ConvertedType> {
     }
 }
 
+// ----------------------------------------------------------------------
+// parquet::BloomFilterHash <=> BloomFilterHash conversion
+
+impl From<parquet::BloomFilterHash> for BloomFilterHash {
+    fn from(value: parquet::BloomFilterHash) -> Self {
+        match value {
+            parquet::BloomFilterHash::XXHASH(_) => BloomFilterHash::XXHASH,
+        }
+    }
+}
+
+impl From<BloomFilterHash> for parquet::BloomFilterHash {

Review Comment:
   BTW this is not anything you did, but I found the use if `use crate::format 
as parquet` very confusing -- given how many things are called parquet
   
   I would personally suggest we use `crate::format` explicitly everywhere for 
clarity



##########
parquet/src/basic.rs:
##########
@@ -817,6 +877,90 @@ impl From<ConvertedType> for 
Option<parquet::ConvertedType> {
     }
 }
 
+// ----------------------------------------------------------------------
+// parquet::BloomFilterHash <=> BloomFilterHash conversion
+
+impl From<parquet::BloomFilterHash> for BloomFilterHash {

Review Comment:
   Is the idea that these are transition mappings from the thrift structures 
--> the native Rust structures?
   
   I am hoping that the end stage will be a single `BloomFilterHash` enum in 
the rust parquet crate that is serialized/deserialized directly to thrift by 
your new fancy encoder/decoder. Is this your vision too?



##########
parquet/src/basic.rs:
##########
@@ -26,17 +28,11 @@ use crate::format as parquet;
 
 use crate::errors::{ParquetError, Result};
 
-// Re-export crate::format types used in this module

Review Comment:
   this is the main API change in this PR, right? No more directly exporting 
the thrift definitions



-- 
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.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to