feat: Support parquet bloom filter pruning for decimal128 #8930

Ted-Jiang · 2024-01-21T08:57:29Z

Which issue does this PR close?

related #8880.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Ted-Jiang · 2024-01-21T08:58:11Z

benchmarks/src/parquet_filter.rs

@@ -89,16 +89,19 @@ impl RunOpt {
                pushdown_filters: false,
                reorder_filters: false,
                enable_page_index: false,
+                enable_bloom_filter: false,


Will support soon

Ted-Jiang · 2024-01-21T09:00:37Z

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

-    /// Maps column name to the parquet bloom filter
-    column_sbbf: HashMap<String, Sbbf>,
+    /// Maps column name to the parquet bloom filter and parquet physical type
+    column_sbbf: HashMap<String, (Sbbf, Type)>,


because the sbbf check input is parquet type, so we need know the read parquet physical type when construct check value 🤔 , is there any another way

Ted-Jiang · 2024-01-21T09:03:49Z

datafusion/core/tests/parquet/filter_pushdown.rs

@@ -80,15 +81,15 @@ async fn single_file() {
        // request_method = 'GET'
        .with_filter(col("request_method").eq(lit("GET")))
        .with_pushdown_expected(PushdownExpected::Some)
-        .with_expected_rows(688);
+        .with_expected_rows(745);


@alamb https://github.com/apache/arrow-datafusion/blob/72fb87e27bfaf62580f30fb6e85aed2a22fbe744/test-utils/src/data_gen.rs#L172 I think here every running test should get diff result 🤔 , but from the fact only change the generate method this would change. Is there something i miss?

I am not quite sure what you are asking here @Ted-Jiang . It seems like most of the tests in this file have been updated as well

datafusion/core/tests/parquet/row_group_pruning.rs

alamb · 2024-01-21T15:40:41Z

Thanks @Ted-Jiang -- I hope to review this more carefully tomorrow

alamb

Thanks @Ted-Jiang -- I think this PR is really nicely done.

I didn't quite make it through all the changes in datafusion/core/tests/parquet/filter_pushdown.rs -- would it be possible to avoid changes in the existing tests?

Either by avoiding changes to the data created or maybe by using a different data generator?

alamb · 2024-01-22T21:04:16Z

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

+                    ScalarValue::Int16(Some(v)) => sbbf.check(v),
+                    ScalarValue::Int8(Some(v)) => sbbf.check(v),
+                    ScalarValue::Decimal128(Some(v), p, s) => match parquet_type {
+                        Type::INT32 => {


Is the value of the i128 the same as the value of this i32 (aka are we sure the top 12 bytes are always 0) as this code is ignores those bytes?

The same basic question applies to the Int64 case too (are we sure its top 8 bytes are always zero)

I think add precision check avoid this situation

alamb · 2024-01-22T21:05:56Z

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

+                            //https://github.com/apache/parquet-format/blob/eb4b31c1d64a01088d02a2f9aefc6c17c54cc6fc/Encodings.md?plain=1#L35-L42
+                            // All physical type  are little-endian
+                            let b = (*v as i32).to_le_bytes();
+                            let decimal = Decimal::Int32 {
+                                value: b,
+                                precision: *p as i32,
+                                scale: *s as i32,
+                            };
+                            sbbf.check(&decimal)


I think it would be clearer to use the constructors instead: https://docs.rs/parquet/latest/parquet/data_type/enum.Decimal.html#method.from_i32

Suggested change

//https://github.com/apache/parquet-format/blob/eb4b31c1d64a01088d02a2f9aefc6c17c54cc6fc/Encodings.md?plain=1#L35-L42

// All physical type are little-endian

let b = (*v as i32).to_le_bytes();

let decimal = Decimal::Int32 {

value: b,

precision: *p as i32,

scale: *s as i32,

};

sbbf.check(&decimal)

let decimal = Decimal::from_i32(* v as i32, *p as i32, *s as i32)'

sbbf.check(&decimal)

(the same applies to the other types here as well)

file apache/arrow-rs#5325 to look at the bytes_order issue

datafusion/core/tests/parquet/row_group_pruning.rs

alamb · 2024-01-22T21:09:07Z

datafusion/core/tests/parquet/mod.rs

@@ -66,7 +66,9 @@ enum Scenario {
    Int32Range,
    Float64,
    Decimal,
+    DecimalBloomFilter,


There are only two scenarios here but three encodings (Int32, Int64 and Bytes) handled by the code. Is there some way to over all three encodings?

alamb · 2024-01-22T21:09:58Z

datafusion/core/tests/parquet/filter_pushdown.rs

@@ -80,15 +81,15 @@ async fn single_file() {
        // request_method = 'GET'
        .with_filter(col("request_method").eq(lit("GET")))
        .with_pushdown_expected(PushdownExpected::Some)
-        .with_expected_rows(688);
+        .with_expected_rows(745);


I am not quite sure what you are asking here @Ted-Jiang . It seems like most of the tests in this file have been updated as well

test-utils/src/data_gen.rs

datafusion/core/tests/parquet/filter_pushdown.rs

Ted-Jiang · 2024-01-23T05:04:26Z

@alamb I remove the change of the datafusion/core/tests/parquet/filter_pushdown.rs file #8958, and add decimal precision check

alamb

Looks good to me -- thank you @Ted-Jiang . I had some comment / test suggestions but I think they could be done as a follow on PR too

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

datafusion/core/tests/parquet/mod.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2024-01-24T12:08:19Z

🎉

github-actions bot added the core Core DataFusion crate label Jan 21, 2024

Ted-Jiang commented Jan 21, 2024

View reviewed changes

Ted-Jiang mentioned this pull request Jan 21, 2024

Support bloom filter pruning in parquet benchmark #8931

Open

Ted-Jiang commented Jan 21, 2024

View reviewed changes

datafusion/core/tests/parquet/row_group_pruning.rs Show resolved Hide resolved

Ted-Jiang commented Jan 21, 2024

View reviewed changes

datafusion/core/tests/parquet/row_group_pruning.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Jan 21, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

alamb mentioned this pull request Jan 22, 2024

fix: issue #8922 make row group test more readable #8941

Merged

alamb reviewed Jan 22, 2024

View reviewed changes

Ted-Jiang force-pushed the decimal branch 2 times, most recently from 41a61f0 to f52f5e1 Compare January 23, 2024 03:49

This was referenced Jan 23, 2024

Construct decimal from i32 apache/arrow-rs#5325

Open

Add bloom filter into integration test for parquet predicate pushdown #8958

Open

alamb approved these changes Jan 23, 2024

View reviewed changes

Ted-Jiang and others added 3 commits January 24, 2024 11:00

[Pruning] Support parquet bloom filter pruning for decimal128

1b602d9

rebase

9cf45bb

Apply suggestions from code review

8fc6348

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Ted-Jiang force-pushed the decimal branch from 010ceb4 to 8fc6348 Compare January 24, 2024 03:24

fix test

6dc6b16

Ted-Jiang merged commit 19ca7d2 into apache:main Jan 24, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support parquet bloom filter pruning for decimal128 #8930

feat: Support parquet bloom filter pruning for decimal128 #8930

Ted-Jiang commented Jan 21, 2024

Ted-Jiang Jan 21, 2024

Ted-Jiang Jan 21, 2024

Ted-Jiang Jan 21, 2024

Ted-Jiang Jan 21, 2024 •

edited

Loading

alamb Jan 22, 2024

alamb commented Jan 21, 2024

alamb left a comment

alamb Jan 22, 2024

Ted-Jiang Jan 23, 2024

alamb Jan 22, 2024

Ted-Jiang Jan 23, 2024

alamb Jan 22, 2024

Ted-Jiang Jan 23, 2024

alamb Jan 22, 2024

Ted-Jiang commented Jan 23, 2024 •

edited

Loading

alamb left a comment

alamb commented Jan 24, 2024 •

edited

Loading

feat: Support parquet bloom filter pruning for decimal128 #8930

feat: Support parquet bloom filter pruning for decimal128 #8930

Conversation

Ted-Jiang commented Jan 21, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

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

Ted-Jiang Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 21, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang commented Jan 23, 2024 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jan 24, 2024 • edited Loading

Ted-Jiang Jan 21, 2024 •

edited

Loading

Ted-Jiang commented Jan 23, 2024 •

edited

Loading

alamb commented Jan 24, 2024 •

edited

Loading