Support skip_values in ColumnValueDecoderImpl #2089

thinkharderdev · 2022-07-16T12:26:40Z

Which issue does this PR close?

Closes #2078

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

thinkharderdev · 2022-07-16T12:28:51Z

parquet/src/encodings/decoding.rs

    fn values_left(&self) -> usize {
        self.values_left
    }

    fn encoding(&self) -> Encoding {
        Encoding::DELTA_BINARY_PACKED
    }
+
+    fn skip(&mut self, num_values: usize) -> Result<usize> {
+        let mut buffer = vec![T::T::default(); num_values];


This seems to kind of defeat the purpose but not sure how to skip delta encoded values without it....

thinkharderdev · 2022-07-16T12:29:05Z

parquet/src/encodings/decoding.rs

@@ -922,6 +974,11 @@ impl<T: DataType> Decoder<T> for DeltaByteArrayDecoder<T> {
    fn encoding(&self) -> Encoding {
        Encoding::DELTA_BYTE_ARRAY
    }
+
+    fn skip(&mut self, num_values: usize) -> Result<usize> {
+        let mut buffer = vec![T::T::default(); num_values];


thinkharderdev · 2022-07-16T12:29:33Z

@Ted-Jiang

codecov-commenter · 2022-07-16T13:13:24Z

Codecov Report

Merging #2089 (7657f96) into master (72dada6) will increase coverage by 0.03%.
The diff coverage is 86.59%.

@@            Coverage Diff             @@
##           master    #2089      +/-   ##
==========================================
+ Coverage   83.70%   83.73%   +0.03%     
==========================================
  Files         224      224              
  Lines       58958    59391     +433     
==========================================
+ Hits        49350    49734     +384     
- Misses       9608     9657      +49

Impacted Files	Coverage Δ
parquet/src/column/reader/decoder.rs	`63.38% <0.00%> (-5.86%)`	⬇️
parquet/src/data_type.rs	`75.74% <84.00%> (+0.98%)`	⬆️
parquet/src/encodings/decoding.rs	`88.70% <85.79%> (-0.84%)`	⬇️
parquet/src/encodings/rle.rs	`92.59% <92.77%> (+0.03%)`	⬆️
parquet/src/util/bit_util.rs	`93.70% <92.95%> (-0.11%)`	⬇️
arrow/src/compute/kernels/take.rs	`95.46% <0.00%> (+0.19%)`	⬆️
arrow/src/array/array_union.rs	`90.89% <0.00%> (+0.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72dada6...7657f96. Read the comment docs.

tustvold

Awesome work, only some minor nits

tustvold · 2022-07-16T12:47:21Z

arrow-flight/src/arrow.flight.protocol.rs

-            tonic::Response<tonic::codec::Streaming<super::HandshakeResponse>>,
-            tonic::Status,
-        > {
+                tonic::Response<tonic::codec::Streaming<super::HandshakeResponse>>,


I wonder if you have an out of date lockfile, which is resulting in these changes??

Yeah, that was it. Thanks!

tustvold · 2022-07-16T12:47:46Z

arrow/Cargo.toml

@@ -41,7 +41,7 @@ bench = false
 ahash = { version = "0.7", default-features = false }
 serde = { version = "1.0", default-features = false }
 serde_derive = { version = "1.0", default-features = false }
-serde_json = { version = "1.0", default-features = false, features = ["preserve_order"] }
+serde_json = { version = "1.0", default-features = false, features = ["preserve_order","std"] }


This shouldn't be necessary if you update your lockfile

tustvold · 2022-07-16T12:52:16Z

parquet/src/data_type.rs

+            let bit_reader = decoder.bit_reader.as_mut().unwrap();
+            let num_values = std::cmp::min(num_values, decoder.num_values);
+            let mut buffer = vec![false; num_values];
+            let values_read = bit_reader.get_batch(&mut buffer[..num_values], 1);


BitReader::skip?

tustvold · 2022-07-16T12:53:02Z

parquet/src/data_type.rs

+                #[inline]
+                fn skip(decoder: &mut PlainDecoderDetails, num_values: usize) -> Result<usize> {
+                    let data = decoder.data.as_ref().expect("set_data should have been called");
+                    let num_values = std::cmp::min(num_values, decoder.num_values);


Suggested change

let num_values = std::cmp::min(num_values, decoder.num_values);

let num_values = num_values.min(decoder.num_values);

tustvold · 2022-07-16T12:53:28Z

parquet/src/data_type.rs

+                .data
+                .as_mut()
+                .expect("set_data should have been called");
+            let num_values = std::cmp::min(num_values, decoder.num_values);


Suggested change

let num_values = std::cmp::min(num_values, decoder.num_values);

let num_values = num_values.min(decoder.num_values);

tustvold · 2022-07-16T13:17:30Z

parquet/src/encodings/rle.rs

+            "eee", "fff", "ddd", "eee", "fff", "eee", "fff",
+            "fff",
+        ];
+        let skipped = decoder.skip(4).expect("skipping two values");


Suggested change

let skipped = decoder.skip(4).expect("skipping two values");

let skipped = decoder.skip(4).expect("skipping four values");

tustvold · 2022-07-16T13:19:28Z

parquet/src/util/bit_util.rs

+
+        let mut values_skipped = 0;
+
+        if num_bits > 32 {


We don't need this special case for skip, this exists in get_batch because we only have unpack32 and not unpack64

Ted-Jiang · 2022-07-16T14:47:42Z

parquet/src/encodings/decoding.rs

@@ -355,6 +362,8 @@ impl<T: DataType> Decoder<T> for DictDecoder<T> {
        rle.get_batch_with_dict(&self.dictionary[..], buffer, num_values)
    }

+


nit: useless space.

Ted-Jiang · 2022-07-16T14:50:12Z

parquet/src/encodings/decoding.rs

@@ -291,6 +294,10 @@ impl<T: DataType> Decoder<T> for PlainDecoder<T> {
    fn get(&mut self, buffer: &mut [T::T]) -> Result<usize> {
        T::T::decode(buffer, &mut self.inner)
    }
+


Should we set #[inline] get better perfomance ?🤔

because i fond encoding set_data all with inline

parquet/src/encodings/decoding.rs

Ted-Jiang · 2022-07-16T15:06:34Z

Nice job! 🥳

Co-authored-by: Yang Jiang <jiangyang381@163.com>

ursabot · 2022-07-16T21:21:10Z

Benchmark runs are scheduled for baseline = 13adaa7 and contender = c585544. c585544 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

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate parquet Changes to the parquet crate labels Jul 16, 2022

thinkharderdev commented Jul 16, 2022

View reviewed changes

thinkharderdev changed the title ~~Issue 2078~~ Support skip_values in ColumnValueDecoderImpl Jul 16, 2022

tustvold approved these changes Jul 16, 2022

View reviewed changes

tustvold mentioned this pull request Jul 16, 2022

Make skip value in ByteArrayDecoderDictionary avoid decoding #2088

Closed

thinkharderdev added 3 commits July 16, 2022 09:30

Add skip method to ParquetValueType

3a3da49

Tests

8ae0f26

PR comments

39a872f

thinkharderdev force-pushed the issue-2078 branch from 154982e to 39a872f Compare July 16, 2022 13:40

Ted-Jiang approved these changes Jul 16, 2022

View reviewed changes

Ted-Jiang reviewed Jul 16, 2022

View reviewed changes

parquet/src/encodings/decoding.rs Outdated Show resolved Hide resolved

tustvold removed arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Jul 16, 2022

thinkharderdev and others added 2 commits July 16, 2022 14:49

Update parquet/src/encodings/decoding.rs

a8b910f

Co-authored-by: Yang Jiang <jiangyang381@163.com>

PR comments

7657f96

tustvold merged commit c585544 into apache:master Jul 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support skip_values in ColumnValueDecoderImpl #2089

Support skip_values in ColumnValueDecoderImpl #2089

thinkharderdev commented Jul 16, 2022

thinkharderdev Jul 16, 2022

thinkharderdev Jul 16, 2022

thinkharderdev commented Jul 16, 2022

codecov-commenter commented Jul 16, 2022 •

edited

tustvold left a comment

tustvold Jul 16, 2022

thinkharderdev Jul 16, 2022

tustvold Jul 16, 2022

tustvold Jul 16, 2022

tustvold Jul 16, 2022

tustvold Jul 16, 2022

tustvold Jul 16, 2022

tustvold Jul 16, 2022

Ted-Jiang Jul 16, 2022

Ted-Jiang Jul 16, 2022

Ted-Jiang Jul 16, 2022

Ted-Jiang commented Jul 16, 2022

ursabot commented Jul 16, 2022

	let num_values = std::cmp::min(num_values, decoder.num_values);
	let num_values = num_values.min(decoder.num_values);

	let skipped = decoder.skip(4).expect("skipping two values");
	let skipped = decoder.skip(4).expect("skipping four values");

		@@ -355,6 +362,8 @@ impl<T: DataType> Decoder<T> for DictDecoder<T> {
		rle.get_batch_with_dict(&self.dictionary[..], buffer, num_values)
		}

Support skip_values in ColumnValueDecoderImpl #2089

Support skip_values in ColumnValueDecoderImpl #2089

Conversation

thinkharderdev commented Jul 16, 2022

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

thinkharderdev commented Jul 16, 2022

codecov-commenter commented Jul 16, 2022 • edited

Codecov Report

tustvold 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

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

Ted-Jiang commented Jul 16, 2022

ursabot commented Jul 16, 2022

codecov-commenter commented Jul 16, 2022 •

edited