Expose parquet reader settings using normal DataFusion `ConfigOptions` #3822

alamb · 2022-10-13T16:03:35Z

Which issue does this PR close?

Rationale for this change

I want to test out the parquet filter pushdown on real datasets using datafusion-cli so we can enable it by default -- #3463

I also want an easy way to disable the feature if users find they are getting wrong results

I want to be able to do so via datafusion-cli like:

$ target/debug/datafusion-cli
DataFusion CLI v13.0.0
❯ show all;
+-------------------------------------------------+---------+
| name                                            | setting |
+-------------------------------------------------+---------+
| datafusion.execution.time_zone                  | UTC     |
| datafusion.execution.parquet.pushdown_filters   | false   | <---- Note the option is now visible here
| datafusion.explain.physical_plan_only           | false   |
| datafusion.execution.coalesce_target_batch_size | 4096    |
| datafusion.execution.batch_size                 | 8192    |
| datafusion.execution.coalesce_batches           | true    |
| datafusion.explain.logical_plan_only            | false   |
| datafusion.optimizer.skip_failed_rules          | true    |
| datafusion.optimizer.filter_null_join_keys      | false   |
+-------------------------------------------------+---------+

And then set them like:

$ DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS=true target/debug/datafusion-cli
DataFusion CLI v13.0.0
❯ show all;
+-------------------------------------------------+---------+
| name                                            | setting |
+-------------------------------------------------+---------+
| datafusion.execution.batch_size                 | 8192    |
| datafusion.execution.coalesce_batches           | true    |
| datafusion.explain.logical_plan_only            | false   |
| datafusion.optimizer.filter_null_join_keys      | false   |
| datafusion.execution.parquet.enable_page_index  | false   |
| datafusion.optimizer.skip_failed_rules          | true    |
| datafusion.explain.physical_plan_only           | false   |
| datafusion.execution.time_zone                  | UTC     |
| datafusion.execution.coalesce_target_batch_size | 4096    |
| datafusion.execution.parquet.pushdown_filters   | true    | <---- Note the option is set to true here!!!!
| datafusion.execution.parquet.reorder_filters    | false   |
+-------------------------------------------------+---------+

What changes are included in this PR?

Add three new config settings to ConfigOptions
Thread ConfigOptions down into the FileScanConfig
Remove ParquetScanOptions in favor of these new configs (will comment on the rationale here)

Are there any user-facing changes?

YES: If you used ParquetScanOptions (which I know @thinkharderdev does) the API has changed.

Also, the settings are now visible in the user level documentation

alamb · 2022-10-13T16:04:58Z

benchmarks/src/bin/parquet_filter_pushdown.rs

@@ -109,6 +111,13 @@ async fn main() -> Result<()> {
    Ok(())
 }

+#[derive(Debug, Clone)]
+struct ParquetScanOptions {


this was replicated for the benchmark code as I felt such a struct was the easiest to understand for this matrix strategy

alamb · 2022-10-13T16:05:30Z

datafusion/core/src/config.rs

                 4096,
            ),
+            ConfigDefinition::new_string(


I moved this to be with the other settings

alamb · 2022-10-13T16:06:00Z

datafusion/core/src/config.rs

+    /// Create new ConfigOptions struct, taking values from
+    /// environment variables where possible.
+    ///
+    /// For example, setting `DATAFUSION_EXECUTION_BATCH_SIZE` will


I will add some documentation about this to the datafusion-cli docs as I couldn't find it when I was looking

alamb · 2022-10-13T16:10:22Z

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

@@ -69,43 +72,6 @@ use parquet::file::{
 };
 use parquet::schema::types::ColumnDescriptor;

-#[derive(Debug, Clone, Default)]
-/// Specify options for the parquet scan
-pub struct ParquetScanOptions {


The key change to this PR is removing this structure and instead reading the values from a ConfigOptions that is threaded down.

You can see in this PR there is already a structure for configuring parquet reading (ParquetReadOptions) so I actually think this will make the code less confusing to work with going forward.

alamb · 2022-10-13T16:11:03Z

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

+    /// `ParquetRecordBatchStream`. These filters are applied by the
+    /// parquet decoder to skip unecessairly decoding other columns
+    /// which would not pass the predicate. Defaults to false
+    pub fn with_pushdown_filters(self, pushdown_filters: bool) -> Self {


This just uses the slightly messier config API to get/set the settings

alamb · 2022-10-13T16:11:34Z

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

-        let reorder_predicates = self.scan_options.reorder_predicates;
-        let pushdown_filters = self.scan_options.pushdown_filters;
-        let enable_page_index = self.scan_options.enable_page_index;
+        let reorder_predicates = self.reorder_filters;


I also took the opportunity to change to consistently use the word filters rather than filters and predciates

alamb · 2022-10-13T16:12:58Z

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

-                    .with_pushdown_filters(true)
-                    .with_reorder_predicates(true),
-            );
+            parquet_exec = parquet_exec


Here is a good illustration of how the API changes (I think for the better)

alamb · 2022-10-13T16:13:29Z

datafusion/core/src/execution/options.rs

@@ -160,10 +170,12 @@ pub struct ParquetReadOptions<'a> {
    pub table_partition_cols: Vec<String>,
    /// Should DataFusion parquet reader use the predicate to prune data,
    /// overridden by value on execution::context::SessionConfig
+    // TODO move this into ConfigOptions


I will do this as a follow on PR

thinkharderdev · 2022-10-14T10:43:36Z

Nice! I will review this this evening or tomorrow depending on how the day goes

thinkharderdev

Had a few comments about why we need a shareable ConfigOptions down at the exec layer but overall this is a great improvement

thinkharderdev · 2022-10-15T10:57:46Z

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

+    /// minimize the cost of filter evaluation by reordering the
+    /// predicate [`Expr`]s. If false, the predicates are applied in
+    /// the same order as specified in the query. Defaults to false.
+    pub fn with_reorder_filters(self, reorder_filters: bool) -> Self {


See above, wrapping the options in a Arc<RwLock<_>> seems strange since this is already essentially an owned value.

thinkharderdev · 2022-10-15T11:00:13Z

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

@@ -698,6 +699,7 @@ mod tests {
            projection,
            statistics,
            table_partition_cols,
+            config_options: ConfigOptions::new().into_shareable(),


Not sure I understand why we need the into_shareable here. Seems like this should just be an owned ConfigOptions

I agree it is non obvious -- ConfigOptions was changed to be shareable by @waitingkuo in #3455

Basically the usecase there was so that the configuration was owned by SessionContext but other parts could read it if necessary -- specifically, information_schema.df_settings / SHOW ALL initially. This PR extends the concept so that the settings can be read by the parquet reader

What would you think about moving the mutability handling into ConfigOption so this looks like

config_options: ConfigOptions::new(),

That would hide the details of shared ownership more nicely I think

Yeah, that makes sense. If ConfigOptions is meant to be shareable then it can just hold a Arc<RwLock<HashMap<_>>

Will do as a follow on PR

Filed #3886

PR to clean up: #3909

alamb · 2022-10-18T14:06:46Z

I plan to merge this after it passes CI to keep the process going. I really like the ConfigOptions structure

andygrove

LGTM. Thanks @alamb

github-actions bot added the core Core DataFusion crate label Oct 13, 2022

alamb changed the title ~~Expose parquet reader settings as DataFusion config settings~~ Expose parquet reader settings using normal DataFusion ConfigOptions Oct 13, 2022

alamb commented Oct 13, 2022

View reviewed changes

alamb marked this pull request as ready for review October 13, 2022 16:15

This was referenced Oct 13, 2022

Minor: Sort the output of SHOW ALL by config name #3823

Merged

Enable Parquet Row and Page Filtering by default (WIP) #3828

Closed

alamb requested review from thinkharderdev and tustvold October 14, 2022 10:33

andygrove added the api change Changes the API exposed to users of the crate label Oct 14, 2022

thinkharderdev reviewed Oct 15, 2022

View reviewed changes

andygrove approved these changes Oct 18, 2022

View reviewed changes

alamb added 3 commits October 18, 2022 13:14

Expose parquet reader settings as DataFusion config settings

b9a5bb6

fix logical conflit

f1cdaba

Update tests

900e15f

alamb force-pushed the alamb/expose_parquet_settings branch 2 times, most recently from cc6c3db to 900e15f Compare October 18, 2022 17:18

This was referenced Oct 18, 2022

Consolidate remaining parquet config options into ConfigOptions #3885

Closed

Make ConfigOptions easier to work with #3886

Closed

alamb merged commit 6e0097d into apache:master Oct 19, 2022

alamb mentioned this pull request Oct 26, 2022

Enable parquet filter pushdown by default #3463

Open

6 tasks

alamb deleted the alamb/expose_parquet_settings branch November 5, 2022 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose parquet reader settings using normal DataFusion `ConfigOptions` #3822

Expose parquet reader settings using normal DataFusion `ConfigOptions` #3822

alamb commented Oct 13, 2022 •

edited

Loading

alamb Oct 13, 2022

alamb Oct 13, 2022

alamb Oct 13, 2022

alamb Oct 18, 2022

alamb Oct 13, 2022

alamb Oct 13, 2022

alamb Oct 13, 2022

alamb Oct 13, 2022

alamb Oct 13, 2022

thinkharderdev commented Oct 14, 2022

thinkharderdev left a comment

thinkharderdev Oct 15, 2022

thinkharderdev Oct 15, 2022

alamb Oct 15, 2022

thinkharderdev Oct 15, 2022

alamb Oct 18, 2022

alamb Oct 18, 2022

alamb Oct 20, 2022

alamb commented Oct 18, 2022

andygrove left a comment

Expose parquet reader settings using normal DataFusion ConfigOptions #3822

Expose parquet reader settings using normal DataFusion ConfigOptions #3822

Conversation

alamb commented Oct 13, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkharderdev commented Oct 14, 2022

thinkharderdev 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

alamb commented Oct 18, 2022

andygrove left a comment

Choose a reason for hiding this comment

Expose parquet reader settings using normal DataFusion `ConfigOptions` #3822

Expose parquet reader settings using normal DataFusion `ConfigOptions` #3822

alamb commented Oct 13, 2022 •

edited

Loading