Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune #10051

Ted-Jiang · 2024-04-12T07:13:31Z

and fix NOT NULL prune

Which issue does this PR close?

Closes #9961
Closes #10049.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

and fix NOT NULL prune

datafusion/core/src/physical_optimizer/pruning.rs

Ted-Jiang · 2024-04-12T07:15:08Z

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

-        None
+        // see https://github.com/apache/arrow-rs/blob/91f0b1771308609ca27db0fb1d2d49571b3980d8/parquet/src/file/metadata.rs#L979-L982
+        let mut first_row_index = self
+            .col_offset_indexes


https://github.com/apache/arrow-rs/blob/91f0b1771308609ca27db0fb1d2d49571b3980d8/parquet/src/file/metadata.rs#L979-L982

I think you can do this same calculation without allocating intermediate vec s-- something like this:

let row_count_per_page = self .col_offset_indexes .windows(2) .map(|location| Some(location[1].first_row_index - location[0].first_row_index)); Some(Arc::new(Int64Array::from_iter(row_count_per_page)))

🤔 the name col_offset_indexes is somewhat confusing to me as they are PageLocations --

col_offset_indexes: &'a Vec<PageLocation>,

maybe we could rename that field to page_locations 🤔

@alamb thanks to your always conscientious review and kindly suggestions . 👍

Avoid allocate in b2d3641

In my mind page_locations store the offsets in the column chunk , i think they are the same meaning 🤣

alamb

Thanks @Ted-Jiang -- this is looking good. I think it could be merged as is, though I also have several suggestions that I think will make the code and comments better. We can do so as a follow on PR though.

datafusion/core/src/physical_optimizer/pruning.rs

alamb · 2024-04-12T17:07:04Z

datafusion/core/tests/parquet/mod.rs

@@ -824,9 +863,17 @@ fn create_data_batch(scenario: Scenario) -> Vec<RecordBatch> {
        }
        Scenario::WithNullValues => {


Could you please also add the "some null/some not null" null values case to this scenario as well?

make_int_batches_with_null(5, 1, 6),

alamb · 2024-04-12T17:07:47Z

datafusion/core/tests/parquet/mod.rs

+        }
+        Scenario::WithNullValuesPageLevel => {
+            vec![
+                make_int_batches_with_null(5, 1, 6),


I think this scenario should have at least one batch of all nulls (e.g. make_int_batches_with_nulls(5, 0, 0))

i think this will be prune by row group -> column level

alamb · 2024-04-12T17:09:03Z

datafusion/core/tests/parquet/mod.rs

+    no_null_values_start: usize,
+    no_null_values_end: usize,


I think you might be able to simplify this method by sending in 2 parameters. Right now it looks like it interleaves nulls arbitrarily but really the nulls are always at the start and non nulls are at the end

num_nulls: usize, non_nulls: usize

alamb · 2024-04-12T17:26:06Z

datafusion/core/tests/parquet/page_pruning.rs

+// row_group4: page1(1~5), page2(All Null)
+// total 40 rows
+async fn test_pages_with_null_values() {
+    test_prune(


It took me a while to convince myself that this was actually setting up the scenario as described. I eventually found it here:

https://github.com/alamb/arrow-datafusion/blob/dee926519030301f052dc2c3196e4fbef0da4c47/datafusion/core/tests/parquet/mod.rs#L916-L920

I wonder if it would be possible to add some better comments to test_prune mentioning that the created parquet files have 5 rows per page

i will make another pr to improve this tests

datafusion/core/tests/parquet/page_pruning.rs

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

alamb · 2024-04-15T11:44:27Z

Thanks again @Ted-Jiang -- I took another look through this PR and it is very nice 👌

…L prune (apache#10051) * Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune * fix clippy * Update datafusion/core/src/physical_optimizer/pruning.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/core/tests/parquet/page_pruning.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/core/tests/parquet/page_pruning.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/core/tests/parquet/page_pruning.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/core/tests/parquet/page_pruning.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * remove allocate vec * better way avoid allocate vec * simply expr --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Prune pages are all null in ParquetExec by row_counts

dee9265

and fix NOT NULL prune

Ted-Jiang requested a review from alamb April 12, 2024 07:13

github-actions bot added the core Core DataFusion crate label Apr 12, 2024

Ted-Jiang commented Apr 12, 2024

View reviewed changes

datafusion/core/src/physical_optimizer/pruning.rs Show resolved Hide resolved

Ted-Jiang commented Apr 12, 2024

View reviewed changes

Ted-Jiang changed the title ~~Prune pages are all null in ParquetExec by row_counts~~ Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune Apr 12, 2024

fix clippy

efcf6c3

alamb mentioned this pull request Apr 12, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 #10002

Closed

9 tasks

alamb approved these changes Apr 12, 2024

View reviewed changes

Ted-Jiang and others added 8 commits April 13, 2024 18:34

Update datafusion/core/src/physical_optimizer/pruning.rs

88e62fd

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

Update datafusion/core/tests/parquet/page_pruning.rs

641540c

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

Update datafusion/core/tests/parquet/page_pruning.rs

8d173c6

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

Update datafusion/core/tests/parquet/page_pruning.rs

65dc157

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

Update datafusion/core/tests/parquet/page_pruning.rs

75a6d0a

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

remove allocate vec

7b014a2

better way avoid allocate vec

b2d3641

simply expr

f66cce3

Ted-Jiang merged commit 671cef8 into apache:main Apr 14, 2024

Ted-Jiang mentioned this pull request Apr 17, 2024

[minor] make parquet prune tests more readable #10111

Closed

alamb mentioned this pull request Apr 22, 2024

Support IS NOT NULL predicates in PruningPredicate #9231

Closed

appletreeisyellow mentioned this pull request Apr 24, 2024

WIP(iox-10577): patched df upgrade 202-04-14 influxdata/arrow-datafusion#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune #10051

Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune #10051

Uh oh!

Ted-Jiang commented Apr 12, 2024 •

edited by alamb

Loading

Uh oh!

Uh oh!

Ted-Jiang Apr 12, 2024

Uh oh!

alamb Apr 12, 2024

Uh oh!

Ted-Jiang Apr 13, 2024 •

edited

Loading

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

Uh oh!

alamb Apr 12, 2024

Uh oh!

alamb Apr 12, 2024

Uh oh!

Ted-Jiang Apr 13, 2024

Uh oh!

alamb Apr 12, 2024

Uh oh!

alamb Apr 12, 2024

Uh oh!

Ted-Jiang Apr 13, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 15, 2024

Uh oh!

Uh oh!

		@@ -824,9 +863,17 @@ fn create_data_batch(scenario: Scenario) -> Vec<RecordBatch> {
		}
		Scenario::WithNullValues => {

Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune #10051

Prune pages are all null in ParquetExec by row_counts and fix NOT NULL prune #10051

Uh oh!

Conversation

Ted-Jiang commented Apr 12, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Uh oh!

Ted-Jiang Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ted-Jiang Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ted-Jiang Apr 13, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Apr 12, 2024

Choose a reason for hiding this comment

Uh oh!

Ted-Jiang Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 15, 2024

Uh oh!

Uh oh!

Ted-Jiang commented Apr 12, 2024 •

edited by alamb

Loading

Ted-Jiang Apr 13, 2024 •

edited

Loading

Ted-Jiang Apr 13, 2024 •

edited

Loading