Add metrics for parquet page level skipping #4105

Ted-Jiang · 2022-11-04T10:09:26Z

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

Which issue does this PR close?

Closes #4086 .

Rationale for this change

we can get metric like

ParquetExec: limit=None, partitions=[Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet],
  predicate=l_shipdate_min@0 <= 8037, projection=[l_quantity, l_extendedprice, l_discount, l_tax, l_returnflag, l_linestatus, l_shipdate], metrics=[output_rows=1020000, elapsed_compute=1ns, spill_count=0, spilled_bytes=0, mem_used=0, 
  pushdown_rows_filtered{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=0, 
   page_index_rows_filtered{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=5000000, predicate_evaluation_errors{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=0, row_groups_pruned{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=0, bytes_scanned{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=13198873, num_predicate_creation_errors=0, time_elapsed_scanning=114.532731ms, time_elapsed_processing=3.561757295s, pushdown_eval_time{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=2ns, 

   page_index_eval_time{filename=Users/yangjiang/test-data/1g_tpch_pageIndex/lineitem/part-00000-7d2abab2-a301-4452-9f1d-c641e7f15af4-c000.snappy.parquet}=475.372µs, time_elapsed_opening=58.666766ms] |

What changes are included in this PR?

Are there any user-facing changes?

We i try to create single column with multi pages, i think there is a bug in page size check. but i found apache/arrow-rs#2941 add page row number check(not release).

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

Ted-Jiang · 2022-11-04T10:27:19Z

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

+
+        let metrics = rt.parquet_exec.metrics().unwrap();
+
+        // todo fix this  https://github.com/apache/arrow-rs/issues/2941 release change to row limit.


https://github.com/apache/arrow-rs/pull/2942/files#r1013838557
I think theres is a bug in should_add_data_page self.encoder.num_values() always zero.
So no matter how to set data_pagesize_limit and write_batch_size always return 1 page in on colunn chunk.
🤔 I think someone metion this before.

But from the real world file, this metic works fine😂

Ted-Jiang · 2022-11-04T13:56:49Z

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

@@ -449,7 +449,7 @@ impl FileOpener for ParquetOpener {
            // page index pruning: if all data on individual pages can
            // be ruled using page metadata, rows from other columns
            // with that range can be skipped as well
-            if let Some(row_selection) = enable_page_index
+            if let Some(row_selection) = (enable_page_index && !row_groups.is_empty())


when all rowGroups are pruned by rg_metadata(min max), we do this skip fast

alamb

Looks good to me @Ted-Jiang -- thank you 🎉

Note that apache/arrow-rs#2941 should be included as part of #4039 so hopefully we can fix it soon.

I think it would be fine to either merge this PR as is and fix the test as a follow on, or else wait for #4039 to merge and then update this PR with the better stats.

alamb · 2022-11-04T17:18:11Z

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

@@ -63,13 +67,22 @@ impl ParquetFileMetrics {
        let pushdown_eval_time = MetricBuilder::new(metrics)
            .with_new_label("filename", filename.to_string())
            .subset_time("pushdown_eval_time", partition);
+        let page_index_rows_filtered = MetricBuilder::new(metrics)


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

Ted-Jiang · 2022-11-05T14:19:03Z

@alamb fix ut in use page row limit fix ut. just happened to arrow be released 😂

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

alamb

alamb · 2022-11-07T14:29:30Z

Thanks a lot @Ted-Jiang

ursabot · 2022-11-07T14:31:15Z

Benchmark runs are scheduled for baseline = b7a3331 and contender = 4d23cae. 4d23cae 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

Ted-Jiang force-pushed the issue_4086 branch from a243aff to 46ed8f1 Compare November 4, 2022 10:12

Ted-Jiang marked this pull request as draft November 4, 2022 10:18

github-actions bot added the core Core DataFusion crate label Nov 4, 2022

Add statistics for parquet page level skipping

1c97dc4

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

Ted-Jiang force-pushed the issue_4086 branch from 46ed8f1 to 1c97dc4 Compare November 4, 2022 10:24

Ted-Jiang marked this pull request as ready for review November 4, 2022 10:24

Ted-Jiang commented Nov 4, 2022

View reviewed changes

Ted-Jiang requested a review from alamb November 4, 2022 10:28

Ted-Jiang commented Nov 4, 2022

View reviewed changes

alamb mentioned this pull request Nov 4, 2022

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

Merged

3 tasks

alamb approved these changes Nov 4, 2022

View reviewed changes

Dandandan changed the title ~~Add statistics for parquet page level skipping~~ Add metrics for parquet page level skipping Nov 4, 2022

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions sql SQL Planner labels Nov 5, 2022

use page row limit fix ut

005b101

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

Ted-Jiang force-pushed the issue_4086 branch from 2dd7cdf to 005b101 Compare November 5, 2022 14:16

github-actions bot removed logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules sql SQL Planner labels Nov 5, 2022

Ted-Jiang added 2 commits November 5, 2022 22:35

fix.

649cc93

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

remove todo

12e7e1c

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

Ted-Jiang requested a review from alamb November 7, 2022 02:33

alamb approved these changes Nov 7, 2022

View reviewed changes

alamb merged commit 4d23cae into apache:master Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for parquet page level skipping #4105

Add metrics for parquet page level skipping #4105

Ted-Jiang commented Nov 4, 2022

Ted-Jiang Nov 4, 2022

Ted-Jiang Nov 4, 2022

alamb Nov 4, 2022

Ted-Jiang Nov 4, 2022 •

edited

Loading

alamb left a comment

alamb Nov 4, 2022

Ted-Jiang commented Nov 5, 2022 •

edited

Loading

alamb left a comment

alamb commented Nov 7, 2022

ursabot commented Nov 7, 2022


		let metrics = rt.parquet_exec.metrics().unwrap();

		// todo fix this https://github.com/apache/arrow-rs/issues/2941 release change to row limit.

Add metrics for parquet page level skipping #4105

Add metrics for parquet page level skipping #4105

Conversation

Ted-Jiang commented Nov 4, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Ted-Jiang Nov 4, 2022

Choose a reason for hiding this comment

Ted-Jiang Nov 4, 2022

Choose a reason for hiding this comment

alamb Nov 4, 2022

Choose a reason for hiding this comment

Ted-Jiang Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 4, 2022

Choose a reason for hiding this comment

Ted-Jiang commented Nov 5, 2022 • edited Loading

alamb left a comment

Choose a reason for hiding this comment

alamb commented Nov 7, 2022

ursabot commented Nov 7, 2022

Ted-Jiang Nov 4, 2022 •

edited

Loading

Ted-Jiang commented Nov 5, 2022 •

edited

Loading