[GitHub] [arrow-rs] alamb commented on a diff in pull request #3578: Use native types in PageIndex (#3575)

["alamb (via GitHub)" <gi...@apache.org>]

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


##########
parquet/src/encodings/rle.rs:
##########
@@ -18,8 +18,9 @@
 use std::{cmp, mem::size_of};
 
 use crate::errors::{ParquetError, Result};
+use crate::util::bit_util::from_le_slice;
 use crate::util::{
-    bit_util::{self, from_ne_slice, BitReader, BitWriter, FromBytes},

Review Comment:
   to other reviewers, this PR changes from native endian to little endian (parquet is always little endian) -- more comments on this below



##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1502,7 +1503,7 @@ mod tests {
         //col9->date_string_col: BINARY UNCOMPRESSED DO:0 FPO:332847 SZ:111948/111948/1.00 VC:7300 ENC:BIT_PACKED,RLE,PLAIN ST:[min: 01/01/09, max: 12/31/10, num_nulls: 0]
         assert!(!&page_indexes[0][8].is_sorted());
         if let Index::BYTE_ARRAY(index) = &page_indexes[0][8] {
-            check_bytes_page_index(

Review Comment:
   I confirmed there is exiting coverage for boolean a few lines above (bonus that the interface hasn't changed 👍 )
   
   ![Screenshot 2023-01-23 at 6 59 52 AM](https://user-images.githubusercontent.com/490673/214034587-3601056b-eeb6-4a8c-9def-ed66fefccfcf.png)
   



##########
parquet/src/bin/parquet-index.rs:
##########
@@ -156,12 +156,12 @@ fn print_index<T: std::fmt::Debug>(
             idx, o.offset, o.compressed_page_size, row_count
         );
         match &c.min {
-            Some(m) => print!(", min {:>10?}", m),
+            Some(m) => print!(", min {:>10}", m),

Review Comment:
   👍 



##########
parquet/src/file/statistics.rs:
##########
@@ -181,11 +181,11 @@ pub fn from_thrift(
                     // min/max statistics for INT96 columns.
                     let min = min.map(|data| {
                         assert_eq!(data.len(), 12);
-                        from_ne_slice::<Int96>(&data)
+                        from_le_slice::<Int96>(&data)

Review Comment:
   > For native types, this outputs the data as little endian. Floating point types are encoded in IEEE.
   
   👍 



##########
parquet/src/file/serialized_reader.rs:
##########
@@ -1515,7 +1516,7 @@ mod tests {
         //col10->string_col: BINARY UNCOMPRESSED DO:0 FPO:444795 SZ:45298/45298/1.00 VC:7300 ENC:BIT_PACKED,RLE,PLAIN ST:[min: 0, max: 9, num_nulls: 0]
         assert!(&page_indexes[0][9].is_sorted());
         if let Index::BYTE_ARRAY(index) = &page_indexes[0][9] {
-            check_bytes_page_index(

Review Comment:
   is there any coverage for FIXED_LENGTH_BYTE_ARRAY?



-- 
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-unsubscribe@arrow.apache.org

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