Support user defined `ParquetAccessPlan` in `ParquetExec`, validation to `ParquetAccessPlan::select` #10813

alamb · 2024-06-06T18:53:45Z

This PR looks big but the 85% of it is tests and documentation

Which issue does this PR close?

Rationale for this change

Many query engines / use cases have some sort of a specialized index for data stored in parquet. This index can be used to determine which row groups / selections within a file are needed

However, the DataFusion ParquetExec has no way for users to pass this information in. Instead it tries to prune row groups based on the min/max statistics and other information in the file's metadata.

This PR makes it possible for users to pass in a ParquetAccessPlan added in #10738 to ParquetExec with a starting plan, which is then further pruned based on the file's metadata.

What changes are included in this PR?

Allow users to pass in a ParquetAccessPlan for each PartitionedFile read by ParquetExec
Add error checking to ParquetAccessPlan now that it can be specified by users
Document how this works
Add tests for this new API

Are these changes tested?

Yes, new tests are added

Are there any user-facing changes?

a new API

Here is a complete end to end example of using this API: #10701

…ation to ParquetAccessPlan::select

alamb · 2024-06-06T18:56:31Z

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

@@ -182,6 +183,11 @@ impl ParquetAccessPlan {
    /// is returned for *all* the rows in the row groups that are not skipped.
    /// Thus it includes a `Select` selection for any [`RowGroupAccess::Scan`].
    ///
+    /// # Errors


Since users can now provide a ParquetAccessPlan it is important to do validation on the contents.

While technically we could avoid doing this validation when the selections came from the page pruning, I think it would be a good check to have to catch future bugs rather than subtle wrong results so I chose to always validate

alamb · 2024-06-06T18:57:00Z

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

@@ -366,7 +410,8 @@ mod test {
                    RowSelector::select(10),
                    // selectors from the second row group
                    RowSelector::select(5),
-                    RowSelector::skip(7)
+                    RowSelector::skip(7),


turns out that some of the existing unit tests were actually invalid. However, I think the issues are actually test problem, not actual code problems. All the actual parquet reader tests passed

alamb · 2024-06-06T18:57:23Z

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

@@ -139,7 +139,8 @@ impl FileOpener for ParquetOpener {
            let predicate = pruning_predicate.as_ref().map(|p| p.as_ref());
            let rg_metadata = file_metadata.row_groups();
            // track which row groups to actually read
-            let access_plan = ParquetAccessPlan::new_all(rg_metadata.len());
+            let access_plan =


This is the required plumbing, which I am quite pleased with -- it is quite straightforward now

alamb · 2024-06-06T18:57:54Z

datafusion/core/tests/parquet/mod.rs

-        let mut finder = MetricsFinder { metrics: None };
-        accept(physical_plan.as_ref(), &mut finder).unwrap();
-        let parquet_metrics = finder.metrics.unwrap();
+        let parquet_metrics =


Pulled into a new file so I could reuse it

alamb · 2024-06-06T19:13:54Z

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

+///
+/// For a complete example, see the [`parquet_index_advanced` example]).
+///
+/// [`parquet_index_advanced` example]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/parquet_index_advanced.rs


This will be added in #10701

BTW #10701 (example of how to use this API) is ready for review

alamb · 2024-06-06T19:35:46Z

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

+        }
+
+        // validate all Selections
+        for (idx, (rg, rg_meta)) in self


This is new checking added as users can pass in ParquetAccessPlan and the semantics are quite subtle

Jefffrey

Very good explanation, makes sense to me coming in with no context on the original issue 👍

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

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

Co-authored-by: Jeffrey Vo <[email protected]>

alamb

Thank you @Jefffrey for your review

alamb · 2024-06-09T20:08:36Z

BTW I am happy to make additional corrections as follow on PRs if anyone has additional notes

cc @advancedxy @thinkharderdev @crepererum @NGA-TRAN and @Ted-Jiang @xinlifoobar and @hengfeiyang who reviewed the original PR to create ParquetAccessPlan -- this PR adds the ability to specify a starting access plan for a scan

advancedxy

Thanks @alamb, this is great work. I think the code is pretty good.

Sorry for being late to the party, left some minor style issues comment.

advancedxy · 2024-06-11T08:15:16Z

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

@@ -348,14 +384,22 @@ mod test {
        let access_plan = ParquetAccessPlan::new(vec![
            RowGroupAccess::Scan,
            RowGroupAccess::Selection(
-                vec![RowSelector::select(5), RowSelector::skip(7)].into(),
+                // select / skip all 20 rows in row group 1


Nit: to be consistent with L427 in this file, it would be better to call it as
specifies all 20 rows in row group .

advancedxy · 2024-06-11T08:16:43Z

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

+    fn test_invalid_too_few() {
+        let access_plan = ParquetAccessPlan::new(vec![
+            RowGroupAccess::Scan,
+            // select 12 rows, but row group 1 has 20


Nit: -> specifies 12 rows?

I think the select is referred to as selection, which the following code also includes a skip.

advancedxy · 2024-06-11T08:16:49Z

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

+    fn test_invalid_too_many() {
+        let access_plan = ParquetAccessPlan::new(vec![
+            RowGroupAccess::Scan,
+            // select 22 rows, but row group 1 has only 20


advancedxy · 2024-06-11T08:18:42Z

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

+/// The `ParquetExec` will try and further reduce any provided
+/// `ParquetAccessPlan` further based on the contents of `ParquetMetadata` and


Nit: there are two further in this sentence. How about:

/// The `ParquetExec` will try and reduce any provided /// `ParquetAccessPlan` further based on the contents ...

advancedxy · 2024-06-11T08:22:08Z

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

+
+            // check row group count matches the plan
+            return Ok(access_plan.clone());
+        }


Nit: is it better to add a logging in the else branch?

alamb · 2024-06-12T22:29:52Z

Thanks @alamb, this is great work. I think the code is pretty good.

Sorry for being late to the party, left some minor style issues comment.

No worries -- thank you for the comments. I will make a PR to address them shortly

alamb · 2024-06-13T11:37:37Z

PR with comments: #10896

… to `ParquetAccessPlan::select` (apache#10813) * Allow `ParquetAccessPlan` to be passed in to `ParquetExec`, add validation to ParquetAccessPlan::select * Add test for filtering and user supplied access plan * fix on windows * Apply suggestions from code review Co-authored-by: Jeffrey Vo <[email protected]> --------- Co-authored-by: Jeffrey Vo <[email protected]>

… to `ParquetAccessPlan::select` (apache#10813) (#8) * Allow `ParquetAccessPlan` to be passed in to `ParquetExec`, add validation to ParquetAccessPlan::select * Add test for filtering and user supplied access plan * fix on windows * Apply suggestions from code review --------- Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Jeffrey Vo <[email protected]>

… to `ParquetAccessPlan::select` (apache#10813) * Allow `ParquetAccessPlan` to be passed in to `ParquetExec`, add validation to ParquetAccessPlan::select * Add test for filtering and user supplied access plan * fix on windows * Apply suggestions from code review Co-authored-by: Jeffrey Vo <[email protected]> --------- Co-authored-by: Jeffrey Vo <[email protected]>

github-actions bot added the core Core DataFusion crate label Jun 6, 2024

Allow ParquetAccessPlan to be passed in to ParquetExec, add valid…

38fa45a

…ation to ParquetAccessPlan::select

alamb force-pushed the alamb/user_access_plan branch from 3e839ef to 38fa45a Compare June 6, 2024 19:13

Add test for filtering and user supplied access plan

bdb51b2

alamb changed the title ~~Allow ParquetAccessPlan to be passed in to ParquetExec, add validation to ParquetAccessPlan::select~~ Support user defined ParquetAccessPlan in ParquetExec, validation to ParquetAccessPlan::select Jun 6, 2024

alamb mentioned this pull request Jun 6, 2024

Add ParquetAccessPlan, unify RowGroup selection and PagePruning selection #10738

Merged

alamb commented Jun 6, 2024

View reviewed changes

alamb mentioned this pull request Jun 6, 2024

Add advanced_parquet_index.rs example of index in into parquet files #10701

Merged

fix on windows

6a602de

alamb marked this pull request as ready for review June 6, 2024 20:18

adriangb mentioned this pull request Jun 7, 2024

Example using SQLite via SQLx for secondary indexes datafusion-contrib/datafusion-async-parquet-index#1

Merged

Merge remote-tracking branch 'apache/main' into alamb/user_access_plan

baae282

Jefffrey approved these changes Jun 8, 2024

View reviewed changes

datafusion/core/src/datasource/physical_plan/parquet/opener.rs Outdated Show resolved Hide resolved

datafusion/core/src/datasource/physical_plan/parquet/access_plan.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

7799d65

Co-authored-by: Jeffrey Vo <[email protected]>

alamb commented Jun 9, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/user_access_plan

69509a4

alamb merged commit 9503456 into apache:main Jun 9, 2024
23 checks passed

alamb deleted the alamb/user_access_plan branch June 9, 2024 20:06

advancedxy reviewed Jun 11, 2024

View reviewed changes

alamb mentioned this pull request Jun 13, 2024

Refine ParquetAccessPlan comments and tests #10896

Merged

phillipleblanc mentioned this pull request Jul 8, 2024

Support user defined ParquetAccessPlan in ParquetExec, validation to ParquetAccessPlan::select (#10813) spiceai/datafusion#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support user defined `ParquetAccessPlan` in `ParquetExec`, validation to `ParquetAccessPlan::select` #10813

Support user defined `ParquetAccessPlan` in `ParquetExec`, validation to `ParquetAccessPlan::select` #10813

alamb commented Jun 6, 2024 •

edited

Loading

alamb Jun 6, 2024

alamb Jun 6, 2024 •

edited

Loading

alamb Jun 6, 2024 •

edited

Loading

alamb Jun 6, 2024

alamb Jun 6, 2024

alamb Jun 13, 2024

alamb Jun 6, 2024

Jefffrey left a comment

alamb left a comment

alamb commented Jun 9, 2024

advancedxy left a comment

advancedxy Jun 11, 2024

advancedxy Jun 11, 2024

advancedxy Jun 11, 2024

advancedxy Jun 11, 2024

advancedxy Jun 11, 2024

alamb commented Jun 12, 2024

alamb commented Jun 13, 2024

		/// The `ParquetExec` will try and further reduce any provided
		/// `ParquetAccessPlan` further based on the contents of `ParquetMetadata` and

Support user defined ParquetAccessPlan in ParquetExec, validation to ParquetAccessPlan::select #10813

Support user defined ParquetAccessPlan in ParquetExec, validation to ParquetAccessPlan::select #10813

Conversation

alamb commented Jun 6, 2024 • edited Loading

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

alamb Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Jun 6, 2024 • edited Loading

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

Jefffrey left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jun 9, 2024

advancedxy 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

alamb commented Jun 12, 2024

alamb commented Jun 13, 2024

Support user defined `ParquetAccessPlan` in `ParquetExec`, validation to `ParquetAccessPlan::select` #10813

Support user defined `ParquetAccessPlan` in `ParquetExec`, validation to `ParquetAccessPlan::select` #10813

alamb commented Jun 6, 2024 •

edited

Loading

alamb Jun 6, 2024 •

edited

Loading

alamb Jun 6, 2024 •

edited

Loading