Expose remaining parquet config options into ConfigOptions (try 2) #4427

alamb · 2022-11-29T22:19:37Z

this is a reworked version of #3885

Which issue does this PR close?

This also helps towards #3887 and #4349

Rationale for this change

Make it easier for people to see what parquet config options are available will make it more likely they are used
The more mechanisms that configuration is supplied, the more likely it to confuse people

It turns out options for reading parquet files were able to be set (and possibly) overridden by no less than three different structures! This is confusing, to say the least.

What changes are included in this PR?

move metadata_size_hint, enable_pruning, and merge_schema_metadata to new config options
Make the precidence of the parquet options passed down to the ParquetExec clear

Are there any user-facing changes?

parquet reader settings are visible session wide
overrides that are specified per-table or per ParquetExec are handled consistently as an override to session wide defaults

Previously, depending on which of the APIs was used to create / register / run parquet, the settings might change if you change the session config or they might have been a snapshot based on when you registered the reader

alamb · 2022-11-29T22:20:20Z

benchmarks/src/bin/tpch.rs

@@ -396,7 +396,8 @@ async fn get_table(
            }
            "parquet" => {
                let path = format!("{}/{}", path, table);
-                let format = ParquetFormat::default().with_enable_pruning(true);
+                let format = ParquetFormat::new(ctx.config_options())


As the parquet format now reads defaults from ConfigOptions they need to be passed to the constructor

alamb · 2022-11-29T22:22:23Z

datafusion/core/src/config.rs

+                 One read to fetch the 8-byte parquet footer and  \
+                 another to fetch the metadata length encoded in the footer.",
+                DataType::UInt64,
+                ScalarValue::UInt64(None),


As mentioned by @thinkharderdev on #3885 (comment) we should probably change this default to something reasonable (like 64K) but I would rather do that in a follow on PR

Filed #4459 to track

alamb · 2022-11-29T22:23:07Z

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

+    // Session level configuration
+    config_options: Arc<RwLock<ConfigOptions>>,
+    // local overides
+    enable_pruning: Option<bool>,


By changing these to Option I think it is now clearer that if they are left at default, the (documented) value from ConfigOptions is used

alamb · 2022-11-29T22:24:33Z

datafusion/core/src/execution/context.rs

-        let listing_options = options
-            .parquet_pruning(parquet_pruning)
-            .to_listing_options(target_partitions);
+        let listing_options = options.to_listing_options(&self.state.read().config);


In some ways I think this is cleaner as now the options make it to the parquet reader, where as before there are places like this that copy some (but not all) of the settings around

alamb · 2022-11-29T22:24:51Z

datafusion/core/src/execution/context.rs

@@ -1183,7 +1179,6 @@ impl Default for SessionConfig {
            repartition_joins: true,
            repartition_aggregations: true,
            repartition_windows: true,
-            parquet_pruning: true,


yet another copy of this setting!

alamb · 2022-11-29T22:25:22Z

datafusion/core/src/execution/options.rs

-    /// metadata.  Defaults to true.
-    // TODO move this into ConfigOptions
-    pub skip_metadata: bool,
+    /// Should the parquet reader use the predicate to prune row groups?


again, here make it clear that these settings are overrides to the defaults on the session configuration

alamb · 2022-11-29T22:25:57Z

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

@@ -72,6 +72,16 @@ use super::get_output_ordering;
 /// Execution plan for scanning one or more Parquet partitions
 #[derive(Debug, Clone)]
 pub struct ParquetExec {
+    /// Override for `Self::with_pushdown_filters`. If None, uses


I also added this back in so that the overrides set directly on a ParquetExec do not affect the global configuration options

alamb · 2022-11-29T22:26:29Z

datafusion/core/tests/sql/information_schema.rs

@@ -707,8 +707,11 @@ async fn show_all() {
        "| datafusion.execution.coalesce_batches                     | true    |",
        "| datafusion.execution.coalesce_target_batch_size           | 4096    |",
        "| datafusion.execution.parquet.enable_page_index            | false   |",
+        "| datafusion.execution.parquet.metadata_size_hint           | NULL    |",


🎉 one can now see much more explicitly both 1) what the parquet options are and 2) what their default values are

andygrove

LGTM! Thanks @alamb

…arquet_take2

liukun4515

Thanks @alamb

docs/source/user-guide/configs.md

Co-authored-by: Dan Harris <[email protected]>

…arquet_take2

ursabot · 2022-12-01T16:42:18Z

Benchmark runs are scheduled for baseline = 09aea09 and contender = fb8eeb2. fb8eeb2 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

Expose remaining parquet config options into ConfigOptions (try 2)

ed1030b

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

alamb commented Nov 29, 2022

View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Nov 29, 2022

This was referenced Nov 29, 2022

Consolidate remaining parquet config options into ConfigOptions #3885

Closed

DataFusion Configuration Consolidation #4349

Closed

alamb requested review from tustvold, thinkharderdev and liukun4515 November 29, 2022 22:29

fix: doctests

497bbc2

andygrove approved these changes Nov 30, 2022

View reviewed changes

alamb added 2 commits November 30, 2022 14:28

Merge remote-tracking branch 'apache/master' into alamb/consolidate_p…

42f1598

…arquet_take2

Update config doc

185a0e3

alamb mentioned this pull request Nov 30, 2022

Add documentation for support for skipping Parquet row groups #825

Open

liukun4515 approved these changes Dec 1, 2022

View reviewed changes

thinkharderdev approved these changes Dec 1, 2022

View reviewed changes

docs/source/user-guide/configs.md Show resolved Hide resolved

alamb and others added 4 commits December 1, 2022 06:28

Update docs/source/user-guide/configs.md

53ff1be

Co-authored-by: Dan Harris <[email protected]>

Merge remote-tracking branch 'apache/master' into alamb/consolidate_p…

2ce9f09

…arquet_take2

Update configuration docs

305e4d3

Merge remote-tracking branch 'apache/master' into alamb/consolidate_p…

92d117d

…arquet_take2

alamb mentioned this pull request Dec 1, 2022

Default parquet reader to reading 64K footer #4459

Open

alamb merged commit fb8eeb2 into apache:master Dec 1, 2022

alamb deleted the alamb/consolidate_parquet_take2 branch December 1, 2022 16:42

tustvold mentioned this pull request Dec 21, 2022

Push SessionState into FileFormat (#4349) #4699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose remaining parquet config options into ConfigOptions (try 2) #4427

Expose remaining parquet config options into ConfigOptions (try 2) #4427

alamb commented Nov 29, 2022 •

edited

Loading

alamb Nov 29, 2022

alamb Nov 29, 2022

alamb Dec 1, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

alamb Nov 29, 2022

andygrove left a comment

liukun4515 left a comment

ursabot commented Dec 1, 2022

Expose remaining parquet config options into ConfigOptions (try 2) #4427

Expose remaining parquet config options into ConfigOptions (try 2) #4427

Conversation

alamb commented Nov 29, 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

andygrove left a comment

Choose a reason for hiding this comment

liukun4515 left a comment

Choose a reason for hiding this comment

ursabot commented Dec 1, 2022

alamb commented Nov 29, 2022 •

edited

Loading