Use native types in PageIndex (#3575) #3578

tustvold · 2023-01-20T14:22:56Z

Which issue does this PR close?

Closes #3575
Relates to #3577

Rationale for this change

See tickets

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-01-20T14:23:30Z

parquet/src/bin/parquet-index.rs

@@ -132,7 +132,7 @@ fn compute_row_counts(offset_index: &[PageLocation], rows: i64) -> Vec<i64> {
 }

 /// Prints index information for a single column chunk
-fn print_index<T: std::fmt::Debug>(
+fn print_index<T: std::fmt::Display>(


Relates to #3573 FYI @bmmeijers

The indexes now all implement Display 🎉

Would it make sense to format the types for bytearray/fixedlenbytearray with hex layout (using :x?) by default?

I would be willing to review a PR that altered the display implementation

tustvold · 2023-01-20T14:23:47Z

parquet/src/data_type.rs

-impl FromBytes for Int96 {
-    type Buffer = [u8; 12];
-    fn from_le_bytes(bs: Self::Buffer) -> Self {
-        let mut i = Int96::new();
-        i.set_data(
-            from_le_slice(&bs[0..4]),
-            from_le_slice(&bs[4..8]),
-            from_le_slice(&bs[8..12]),
-        );
-        i
-    }
-    fn from_be_bytes(_bs: Self::Buffer) -> Self {
-        unimplemented!()
-    }
-    fn from_ne_bytes(bs: Self::Buffer) -> Self {
-        let mut i = Int96::new();
-        i.set_data(
-            from_ne_slice(&bs[0..4]),
-            from_ne_slice(&bs[4..8]),
-            from_ne_slice(&bs[8..12]),
-        );
-        i
-    }
-}
-
-// FIXME Needed to satisfy the constraint of many decoding functions but ByteArray does not
-// appear to actual be converted directly from bytes
-impl FromBytes for ByteArray {
-    type Buffer = Vec<u8>;
-    fn from_le_bytes(bs: Self::Buffer) -> Self {
-        ByteArray::from(bs)
-    }
-    fn from_be_bytes(_bs: Self::Buffer) -> Self {
-        unreachable!()
-    }
-    fn from_ne_bytes(bs: Self::Buffer) -> Self {
-        ByteArray::from(bs)
-    }
-}
-
-impl FromBytes for FixedLenByteArray {
-    type Buffer = Vec<u8>;
-
-    fn from_le_bytes(bs: Self::Buffer) -> Self {
-        Self(ByteArray::from(bs))
-    }
-    fn from_be_bytes(_bs: Self::Buffer) -> Self {
-        unreachable!()
-    }
-    fn from_ne_bytes(bs: Self::Buffer) -> Self {
-        Self(ByteArray::from(bs))
-    }
-}


These have been moved to live alongside the other FromBytes implementations

tustvold · 2023-01-20T14:24:14Z

parquet/src/file/serialized_reader.rs

@@ -1575,20 +1576,6 @@ mod tests {
        });
    }

-    fn check_bytes_page_index(


We no longer need to treat different index types differently 🎉

tustvold · 2023-01-20T14:32:51Z

parquet/src/util/bit_util.rs

-    let mut b = T::Buffer::default();
-    {
-        let b = b.as_mut();
-        let bs = &bs[..b.len()];
-        b.copy_from_slice(bs);
-    }


This logic was problematic as for ByteArray and FixedLenByteArray use Vec as Self::Buffer, this would then have a length of 0 and so wouldn't copy anything across...

tustvold · 2023-01-20T15:01:01Z

parquet/src/util/bit_util.rs

-            fn from_be_bytes(bs: Self::Buffer) -> Self {
-                <$ty>::from_be_bytes(bs)
-            }
-            fn from_ne_bytes(bs: Self::Buffer) -> Self {
-                <$ty>::from_ne_bytes(bs)
-            }


Data in parquet is always stored as little endian - https://github.com/apache/parquet-format/blob/master/Encodings.md#plain-plain--0

The use of ne_bytes was actually incorrect

tustvold · 2023-01-20T15:02:44Z

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)


The data is little endian, not native endian - https://github.com/apache/parquet-format/blob/master/Encodings.md#plain-plain--0

For native types, this outputs the data as little endian. Floating point types are encoded in IEEE.

👍

tustvold · 2023-01-20T15:03:11Z

parquet/src/util/bit_util.rs

    }
-    T::from_le_bytes(b)
 }

 pub trait FromBytes: Sized {


Note: this trait is not part of the public API

tustvold · 2023-01-20T15:03:27Z

parquet/src/file/page_index/index.rs

+    BOOLEAN(NativeIndex<bool>),
    INT32(NativeIndex<i32>),
    INT64(NativeIndex<i64>),
    INT96(NativeIndex<Int96>),
    FLOAT(NativeIndex<f32>),
    DOUBLE(NativeIndex<f64>),
-    BYTE_ARRAY(ByteArrayIndex),
-    FIXED_LEN_BYTE_ARRAY(ByteArrayIndex),
+    BYTE_ARRAY(NativeIndex<ByteArray>),
+    FIXED_LEN_BYTE_ARRAY(NativeIndex<ByteArray>),


This is the breaking change

tustvold · 2023-01-20T15:09:13Z

parquet/src/util/bit_util.rs

-        let bs = &bs[..b.len()];
-        b.copy_from_slice(bs);
+fn array_from_slice<const N: usize>(bs: &[u8]) -> Result<[u8; N]> {
+    // Need to slice as may be called with zero-padded values


I've confirmed this optimised as you would hope

https://rust.godbolt.org/z/4WPdn4j8j

alamb · 2023-01-20T15:11:53Z

cc @Ted-Jiang

alamb

I am not a super expert in this area but I reviewed the code and test changes / coverage carefully and it looks 👍 to me

The only thing I think worth doing is to review the coverage for FIXED_LEN_BYTE_ARRAY in parquet/src/file/serialized_reader.rs (this PR didn't change the coverage, but it seems like there may be missing statistics coverage for that type)

alamb · 2023-01-23T11:54:18Z

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),


alamb · 2023-01-23T11:56:32Z

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)


For native types, this outputs the data as little endian. Floating point types are encoded in IEEE.

👍

alamb · 2023-01-23T11:57:53Z

parquet/src/encodings/rle.rs

 use crate::util::{
-    bit_util::{self, from_ne_slice, BitReader, BitWriter, FromBytes},


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

alamb · 2023-01-23T12:00:25Z

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(


I confirmed there is exiting coverage for boolean a few lines above (bonus that the interface hasn't changed 👍 )

alamb · 2023-01-23T12:00:57Z

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(


is there any coverage for FIXED_LENGTH_BYTE_ARRAY?

ursabot · 2023-01-23T16:52:25Z

Benchmark runs are scheduled for baseline = 892a803 and contender = de381ec. de381ec is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Use native types in PageIndex (apache#3575)

06823dc

tustvold added the api-change Changes to the arrow API label Jan 20, 2023

github-actions bot added the parquet Changes to the parquet crate label Jan 20, 2023

tustvold commented Jan 20, 2023

View reviewed changes

tustvold marked this pull request as draft January 20, 2023 14:33

Cleanup endianess

bb5d20f

tustvold commented Jan 20, 2023

View reviewed changes

Format

6f6b14c

tustvold marked this pull request as ready for review January 20, 2023 15:05

tustvold commented Jan 20, 2023

View reviewed changes

Cleanup test

b9096c7

tustvold requested a review from alamb January 22, 2023 09:14

alamb approved these changes Jan 23, 2023

View reviewed changes

Add FixedLenByteArray test

8be1ea5

tustvold merged commit de381ec into apache:master Jan 23, 2023

tustvold mentioned this pull request Apr 27, 2023

Remove Type from NativeIndex #4146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use native types in PageIndex (#3575) #3578

Use native types in PageIndex (#3575) #3578

tustvold commented Jan 20, 2023

tustvold Jan 20, 2023

bmmeijers Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

alamb Jan 23, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

tustvold Jan 20, 2023

alamb commented Jan 20, 2023

alamb left a comment

alamb Jan 23, 2023

alamb Jan 23, 2023

alamb Jan 23, 2023

alamb Jan 23, 2023

alamb Jan 23, 2023

ursabot commented Jan 23, 2023

		use crate::util::{
		bit_util::{self, from_ne_slice, BitReader, BitWriter, FromBytes},

Use native types in PageIndex (#3575) #3578

Use native types in PageIndex (#3575) #3578

Conversation

tustvold commented Jan 20, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 20, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Jan 23, 2023