add bench: decimal with byte array and fixed length byte array #2529

liukun4515 · 2022-08-19T15:25:22Z

Which issue does this PR close?

part of #2388

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2022-08-19T15:42:42Z

parquet/src/arrow/mod.rs

@@ -122,7 +122,7 @@
 experimental!(mod array_reader);
 pub mod arrow_reader;
 pub mod arrow_writer;
-mod buffer;
+experimental!(mod buffer);


Can we avoid this, experimental, I really don't want buffer exposed externally

after your refactor #2528, I think we can use some public api like read_array_reader to create the reader in the benchmark.

After remove #2529 (comment) from your suggestion, we can avoid using the buffer crate in benchmark code

tustvold · 2022-08-19T15:43:19Z

parquet/benches/arrow_reader.rs

+            match data_type {
+                DataType::Decimal128(precision, scale) => {
+                    // read decimal data from parquet binary physical type
+                    let convert = DecimalByteArrayConvert::new(


FYI these functions are being removed... #2528

thanks for your remind, after #2528 merged, and refactor this pr and review it.

When the #2425 can be reviewed?

I need reviews on the PRs it builds on, that's the only thing holding it up being ready for review

tustvold · 2022-08-19T15:45:13Z

parquet/src/data_type.rs

@@ -1235,7 +1241,7 @@ impl FromBytes for ByteArray {
 }

 impl FromBytes for FixedLenByteArray {
-    type Buffer = [u8; 8];
+    type Buffer = Vec<u8>;

    fn from_le_bytes(bs: Self::Buffer) -> Self {
        Self(ByteArray::from(bs.to_vec()))


This is performing an unnecessary clone now

tustvold · 2022-08-19T15:47:47Z

I think this will serve as a nice validation of #2528 - the performance should be significantly improved

liukun4515 · 2022-08-19T15:58:51Z

I think this will serve as a nice validation of #2528 - the performance should be significantly improved

hope so, I will review your refactor work tomorrow.

liukun4515 · 2022-08-21T07:48:44Z

I think this will serve as a nice validation of #2528 - the performance should be significantly improved

Hi @tustvold this pr #2522 will improve the performance?
I can't find the point where to improve the performance in #2522 refactor to remove the DecimalByteArrayConvert?

liukun4515 · 2022-08-21T07:58:10Z

It's better to remove the complex_reader from the array reader @tustvold
After your refactor, the type of parquet array reader will be matched with the parquet physical type BOOL,INT32,INT64,INT96,BYTE_ARRAY,FIXED_LENGTH_BYTE_ARRAY. ❤️

tustvold · 2022-08-21T08:06:38Z

I can't find the point where to improve the performance in #2522 refactor to remove the DecimalByteArrayConvert?

CompkexObjectArrayReader reads to the row format, i.e. separately boxed ByteArray for each row. ByteArrayReader reads to a contiguous arrow buffer. It also has an optimised path for reading non-nested definition levels. It should therefore be significantly faster, not to mention correctly handling nesting

See #1082

liukun4515 · 2022-08-29T10:30:42Z

great improvement for the decimal reader using the fixed length byte array after the refactor #2528

arrow_array_reader/FIXED_LENGTH_BYTE_ARRAY/Decimal128Array/plain encoded, mandatory, no NULLs
                        time:   [453.71 us 462.25 us 472.03 us]
                        change: [-57.944% -57.150% -56.305%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  9 (9.00%) high mild
  1 (1.00%) high severe
arrow_array_reader/FIXED_LENGTH_BYTE_ARRAY/Decimal128Array/plain encoded, optional, no NULLs
                        time:   [438.91 us 442.14 us 445.72 us]
                        change: [-51.955% -51.561% -51.191%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) high mild
  3 (3.00%) high severe
arrow_array_reader/FIXED_LENGTH_BYTE_ARRAY/Decimal128Array/plain encoded, optional, half NULLs
                        time:   [590.60 us 595.71 us 601.33 us]
                        change: [-31.035% -30.479% -29.836%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)

@tustvold

…ed_array_#2388

liukun4515 · 2022-08-29T10:40:59Z

@tustvold PTAL
just add benchmark for decimal reader using byte array or fixed length byte array.

ursabot · 2022-08-29T11:41:28Z

Benchmark runs are scheduled for baseline = c6e7680 and contender = 81f1f81. 81f1f81 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

liukun4515 · 2022-08-31T13:18:14Z

BYTE_ARRAY

arrow_array_reader/BYTE_ARRAY/Decimal128Array/plain encoded, mandatory, no NULLs
                        time:   [521.98 us 527.20 us 532.82 us]
                        change: [-64.921% -64.209% -63.473%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  7 (7.00%) high mild
  2 (2.00%) high severe
arrow_array_reader/BYTE_ARRAY/Decimal128Array/plain encoded, optional, no NULLs
                        time:   [547.52 us 559.31 us 575.61 us]
                        change: [-60.657% -59.882% -58.990%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  6 (6.00%) high mild
  2 (2.00%) high severe
arrow_array_reader/BYTE_ARRAY/Decimal128Array/plain encoded, optional, half NULLs
                        time:   [637.63 us 643.18 us 649.31 us]
                        change: [-40.139% -39.311% -38.471%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)

liukun4515 mentioned this pull request Aug 19, 2022

DecimalArray Benchmarks #2388

Closed

github-actions bot added the parquet Changes to the parquet crate label Aug 19, 2022

tustvold reviewed Aug 19, 2022

View reviewed changes

add bench: decimal with byte array and fixed length byte array

853e6a1

Merge remote-tracking branch 'upstream/master' into bench_decimal_fix…

862cf28

…ed_array_#2388

liukun4515 force-pushed the bench_decimal_fixed_array_#2388 branch from a038bba to 862cf28 Compare August 29, 2022 10:37

change comments

9df9192

liukun4515 requested a review from tustvold August 29, 2022 10:43

tustvold approved these changes Aug 29, 2022

View reviewed changes

tustvold merged commit 81f1f81 into apache:master Aug 29, 2022

liukun4515 deleted the bench_decimal_fixed_array_#2388 branch August 30, 2022 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add bench: decimal with byte array and fixed length byte array #2529

add bench: decimal with byte array and fixed length byte array #2529

liukun4515 commented Aug 19, 2022

tustvold Aug 19, 2022

liukun4515 Aug 19, 2022 •

edited

Loading

liukun4515 Aug 19, 2022

tustvold Aug 19, 2022 •

edited

Loading

liukun4515 Aug 19, 2022

tustvold Aug 19, 2022

tustvold Aug 19, 2022

liukun4515 Aug 19, 2022

tustvold commented Aug 19, 2022

liukun4515 commented Aug 19, 2022

liukun4515 commented Aug 21, 2022

liukun4515 commented Aug 21, 2022 •

edited

Loading

tustvold commented Aug 21, 2022 •

edited

Loading

liukun4515 commented Aug 29, 2022

liukun4515 commented Aug 29, 2022

ursabot commented Aug 29, 2022

liukun4515 commented Aug 31, 2022

add bench: decimal with byte array and fixed length byte array #2529

add bench: decimal with byte array and fixed length byte array #2529

Conversation

liukun4515 commented Aug 19, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold Aug 19, 2022

Choose a reason for hiding this comment

liukun4515 Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

liukun4515 Aug 19, 2022

Choose a reason for hiding this comment

tustvold Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

liukun4515 Aug 19, 2022

Choose a reason for hiding this comment

tustvold Aug 19, 2022

Choose a reason for hiding this comment

tustvold Aug 19, 2022

Choose a reason for hiding this comment

liukun4515 Aug 19, 2022

Choose a reason for hiding this comment

tustvold commented Aug 19, 2022

liukun4515 commented Aug 19, 2022

liukun4515 commented Aug 21, 2022

liukun4515 commented Aug 21, 2022 • edited Loading

tustvold commented Aug 21, 2022 • edited Loading

liukun4515 commented Aug 29, 2022

liukun4515 commented Aug 29, 2022

ursabot commented Aug 29, 2022

liukun4515 commented Aug 31, 2022

liukun4515 Aug 19, 2022 •

edited

Loading

tustvold Aug 19, 2022 •

edited

Loading

liukun4515 commented Aug 21, 2022 •

edited

Loading

tustvold commented Aug 21, 2022 •

edited

Loading