Support pushdown multi-columns in PageIndex pruning. #3967

Ted-Jiang · 2022-10-26T10:17:54Z

Signed-off-by: yangjiang yangjiang@ebay.com

Which issue does this PR close?

Closes #3834.
Closes #4002

Rationale for this change

What changes are included in this PR?

Thanks the original pic from @alamb ❤️

I tried to draw a diagram to illustrate what I think is going on:

┏━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ 
   ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─   ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─   ┃
┃     ┌──────────────┐  │     ┌──────────────┐  │  ┃
┃  │  │              │     │  │              │     ┃
┃     │              │  │     │     Page     │  │   
   │  │              │     │  │      3       │     ┃
┃     │              │  │     │   min: "A"   │  │  ┃
┃  │  │              │     │  │   max: "C"   │     ┃
┃     │     Page     │  │     │ first_row: 0 │  │   
   │  │      1       │     │  │              │     ┃
┃     │   min: 10    │  │     └──────────────┘  │  ┃
┃  │  │   max: 20    │     │  ┌──────────────┐     ┃
┃     │ first_row: 0 │  │     │              │  │   
   │  │              │     │  │     Page     │     ┃
┃     │              │  │     │      4       │  │  ┃
┃  │  │              │     │  │   min: "D"   │     ┃
┃     │              │  │     │   max: "G"   │  │   
   │  │              │     │  │first_row: 100│     ┃
┃     └──────────────┘  │     │              │  │  ┃
┃  │  ┌──────────────┐     │  │              │     ┃
┃     │              │  │     └──────────────┘  │   
   │  │     Page     │     │  ┌──────────────┐     ┃
┃     │      2       │  │     │              │  │  ┃
┃  │  │   min: 30    │     │  │     Page     │     ┃
┃     │   max: 40    │  │     │      5       │  │   
   │  │first_row: 200│     │  │   min: "H"   │     ┃
┃     │              │  │     │   max: "Z"   │  │  ┃
┃  │  │              │     │  │first_row: 250│     ┃
┃     └──────────────┘  │     │              │  │   
   │                       │  └──────────────┘     ┃
┃   ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┘   ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┘  ┃
┃       ColumnChunk            ColumnChunk         ┃
┃            A                      B               
 ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━━ ━━┛
                                                    
       total rows: 300

Multi Column Predicates

Given the predicate A > 35 AND B = "F":

Using A > 35, we can rule out page 1 using statistics as before, get selector(200~300)

Using the B = "F" part, we could rule out pages 3 and 5 (row_index 0->99 and row_index 250->onward) , get selector(100~244)

Finally do the intersection get selector(200~244)

Are there any user-facing changes?

Ted-Jiang · 2022-10-26T10:19:21Z

datafusion/core/src/physical_plan/file_format/parquet.rs

-    offset_indexes: &'a Vec<Vec<PageLocation>>,
-    parquet_schema: &'a Schema,
-    col_id: usize,
+    col_page_indexes: &'a Index,


simplify the PagesPruningStatistics no need pass all cols index!

Ted-Jiang · 2022-10-26T10:24:03Z

datafusion/core/src/physical_plan/file_format/parquet.rs

+//
+// returned: NNNNNNNNY
+// set `need_combine` true will combine result: Select(2) + Select(1) + Skip(2) -> Select(3) + Skip(2)
+pub(crate) fn intersect_row_selection(


Will move this to arrow-rs

Filed apache/arrow-rs#3003

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang · 2022-10-29T16:07:07Z

datafusion/core/src/physical_plan/file_format/parquet.rs

                metrics.predicate_evaluation_errors.add(1);
+                return Ok(vec![RowSelector::select(group.num_rows() as usize)]);


@alamb related to #4002 add one fallback, which select all rows.

got

❯ select count(*) from foo where container = 'backend_container_1'; [2022-10-29T15:51:31Z ERROR datafusion::physical_plan::file_format::parquet] Error evaluating page index predicate values Error during planning: Can not create statistics record batch: Invalid argument error: Column 'container_min' is declared as non-nullable but contains null values +-----------------+ | COUNT(UInt8(1)) | +-----------------+ | 15963 | +-----------------+ 1 row in set. Query took 0.035 seconds.

cool! I hope to have #3976 ready for review later today -- with that additional testing coverage I will feel pretty good about this particular optimization 🎉

I verified that this PR fixes #4002 -- I have also prepared a PR to enable page filtering in the integration test #4062 which we can run when this PR is merged

Signed-off-by: yangjiang <yangjiang@ebay.com>

alamb · 2022-11-01T13:41:37Z

Sorry @Ted-Jiang -- i will review this PR today

alamb · 2022-11-01T16:53:43Z

@Ted-Jiang - I took the liberty of merging up from master and fixing a merge conflict

alamb

This looks great @Ted-Jiang 🦾 🏆 -- I think it can be merged as is.

I am impressed with the tests in this PR and I also verified that this optimization passes my parquet predicate ~~torture~~ integration test #4062 that found #4002

I have some follow up suggestions but I think we can do them as follow on PRs if you prefer

Other items to do:

Add statistics to verify and report on the efficiency of page index pruning (I will file a follow on ticket)
Add some more variations to the parquet predicate integration tests to ensure multiple pages are being used (will file a follow on ticket)

datafusion/core/src/physical_plan/file_format/parquet.rs

alamb · 2022-11-01T17:08:36Z

datafusion/core/src/physical_plan/file_format/parquet.rs

+                                    }),
+                                );
+                            } else {
+                                // fallback select all rows


datafusion/core/src/physical_plan/file_format/parquet.rs

alamb · 2022-11-01T17:31:45Z

datafusion/core/src/physical_plan/file_format/parquet.rs

+                }
+            }
+            _ => {
+                if a.row_count < b.row_count {


Shouldn't this be taking which one of a or b was skipped (rather than just the one that was smaller)? I tried messing around and trying to write some test that failed, but was not able to

I think two element at least one is skip, we can sure about skip the min len is correct🤔

alamb · 2022-11-01T17:32:59Z

datafusion/core/tests/parquet_page_index_pruning.rs

+
+    // create filter (year = 2009 and id = 1) or (year = 2010)
+    // this filter use two columns will not push down
+    // todo but after use CNF rewrite it could rewrite to (year = 2009 or  year = 2010) and (id = 1 or year = 2010)


Not sure about the CNF rewrite comment here

Maybe we file a ticket do some tests.

I am not sure what tests you have in mind here so I did not file a ticket -- perhaps you could add to one of the existing tickets (#4087 ?) or file a new one?

Ted-Jiang · 2022-11-02T02:49:57Z

I took the liberty of merging up from master and fixing a merge conflict

Thanks for the helping!😄

Add statistics to verify and report on the efficiency of page index pruning (I will file a follow on ticket)

Add some more variations to the parquet predicate integration tests to ensure multiple pages are being used (will file a follow on ticket)

Agree! will try to familiar with the IT, support these✌️

Signed-off-by: yangjiang <yangjiang@ebay.com>

alamb · 2022-11-02T16:53:47Z

Looks good -- thanks @Ted-Jiang -- I'll merge this in and file some follow on tickets for the remaining items

alamb · 2022-11-02T17:09:44Z

I filed #4085 to track enabling this feature by default

alamb · 2022-11-02T17:13:42Z

Filed #4086 to track adding statistics

alamb · 2022-11-02T17:17:17Z

Filed #4087 for increasing test coverage

ursabot · 2022-11-02T17:32:59Z

Benchmark runs are scheduled for baseline = 065a478 and contender = 1a5f6ab. 1a5f6ab 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-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-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

* Support pushdown multi-columns in PageIndex pruning. Signed-off-by: yangjiang <yangjiang@ebay.com> * fix test Signed-off-by: yangjiang <yangjiang@ebay.com> * fix comments Signed-off-by: yangjiang <yangjiang@ebay.com> * avoid extract predicates when enable is false Signed-off-by: yangjiang <yangjiang@ebay.com> Signed-off-by: yangjiang <yangjiang@ebay.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added the core Core datafusion crate label Oct 26, 2022

Ted-Jiang commented Oct 26, 2022

View reviewed changes

Support pushdown multi-columns in PageIndex pruning.

e48cf9c

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang force-pushed the issue_3834 branch from 61fbb5b to e48cf9c Compare October 26, 2022 10:34

Ted-Jiang marked this pull request as draft October 26, 2022 10:36

Ted-Jiang commented Oct 29, 2022

View reviewed changes

fix test

ab74e46

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang force-pushed the issue_3834 branch from 2315e65 to ab74e46 Compare October 29, 2022 16:09

Ted-Jiang marked this pull request as ready for review October 29, 2022 16:25

Merge remote-tracking branch 'apache/master' into issue_3834

002d7d6

alamb mentioned this pull request Nov 1, 2022

Enable tests for page index filtering in parquet filter pushdown test #4062

Merged

3 tasks

alamb approved these changes Nov 1, 2022

View reviewed changes

Ted-Jiang added 2 commits November 2, 2022 11:52

fix comments

efbf1f8

Signed-off-by: yangjiang <yangjiang@ebay.com>

avoid extract predicates when enable is false

54c3a7d

Signed-off-by: yangjiang <yangjiang@ebay.com>

alamb approved these changes Nov 2, 2022

View reviewed changes

alamb mentioned this pull request Nov 2, 2022

Add RowSelection::intersection apache/arrow-rs#3003

Closed

alamb merged commit 1a5f6ab into apache:master Nov 2, 2022

alamb mentioned this pull request Nov 2, 2022

Minor: Add arrow-rs ticket reference and turn some comments into docstrings #4088

Merged

alamb mentioned this pull request Nov 3, 2022

minor: Split parquet reader up into smaller modules #4099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support pushdown multi-columns in PageIndex pruning. #3967

Support pushdown multi-columns in PageIndex pruning. #3967

Ted-Jiang commented Oct 26, 2022 •

edited by alamb

Ted-Jiang Oct 26, 2022

Ted-Jiang Oct 26, 2022

alamb Nov 2, 2022

Ted-Jiang Oct 29, 2022

alamb Oct 31, 2022

alamb Nov 1, 2022

alamb commented Nov 1, 2022

alamb commented Nov 1, 2022

alamb left a comment

alamb Nov 1, 2022

alamb Nov 1, 2022

Ted-Jiang Nov 2, 2022

alamb Nov 2, 2022

alamb Nov 1, 2022

Ted-Jiang Nov 2, 2022

alamb Nov 2, 2022

Ted-Jiang commented Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

ursabot commented Nov 2, 2022

		metrics.predicate_evaluation_errors.add(1);
		return Ok(vec![RowSelector::select(group.num_rows() as usize)]);

Support pushdown multi-columns in PageIndex pruning. #3967

Support pushdown multi-columns in PageIndex pruning. #3967

Conversation

Ted-Jiang commented Oct 26, 2022 • edited by alamb

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Multi Column Predicates

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

alamb commented Nov 1, 2022

alamb commented Nov 1, 2022

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 Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

alamb commented Nov 2, 2022

ursabot commented Nov 2, 2022

Ted-Jiang commented Oct 26, 2022 •

edited by alamb