Consolidate remaining parquet config options into ConfigOptions #3885

alamb · 2022-10-18T17:20:19Z

~~Draft as it builds on #3822~~

Which issue does this PR close?

Rationale for this change

Make it easier for people to see what 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.

This is especially important to us as we move to use more and more of the parquet code directly in IOx (e.g. https://github.com/influxdata/influxdb_iox/issues/5887 and https://github.com/influxdata/influxdb_iox/issues/5897)

This also helps towards #3887

What changes are included in this PR?

move metadata_size_hint, enable_pruning, and merge_schema_metadata to new config options
Remove redundancy and config overrides

Are there any user-facing changes?

The main change is that now the parquet reader settings are session wide.

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-10-18T17:22:30Z

There are still some tests and code updates required for this PR

alamb · 2022-10-20T16:44:08Z

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

@@ -52,59 +57,70 @@ pub const DEFAULT_PARQUET_EXTENSION: &str = ".parquet";
 /// The Apache Parquet `FileFormat` implementation
 #[derive(Debug)]
 pub struct ParquetFormat {
-    enable_pruning: bool,


Here was one copy of (some) of the settings

alamb · 2022-10-20T16:44:43Z

datafusion/core/src/execution/context.rs

@@ -1142,8 +1146,6 @@ pub struct SessionConfig {
    /// Should DataFusion repartition data using the partition keys to execute window functions in
    /// parallel using the provided `target_partitions` level
    pub repartition_windows: bool,
-    /// Should DataFusion parquet reader using the predicate to prune data
-    pub parquet_pruning: bool,


here is a second copy of one of the settings

alamb · 2022-10-20T16:44:54Z

datafusion/core/src/execution/options.rs

@@ -168,56 +170,31 @@ pub struct ParquetReadOptions<'a> {
    pub file_extension: &'a str,
    /// Partition Columns
    pub table_partition_cols: Vec<String>,
-    /// Should DataFusion parquet reader use the predicate to prune data,


here is a third copy, again of some of the settings

alamb · 2022-10-20T16:45:13Z

datafusion/core/tests/sql/information_schema.rs

@@ -702,8 +702,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    |",


defaults are now even clearer!

alamb · 2022-10-20T16:50:53Z

datafusion/core/tests/sql/parquet_schema.rs

@@ -141,6 +139,13 @@ async fn schema_merge_can_preserve_metadata() {
    let table_path = table_dir.to_str().unwrap().to_string();

    let ctx = SessionContext::new();
+
+    // explicitly disable schema clearing
+    ctx.config_options()


Ideally we will have a nicer API for this: #3908

tustvold · 2022-10-24T01:51:22Z

datafusion/core/src/config.rs

+                 bytes of the parquet file optimistically. If not specified, two read are required: \
+                 One read to fetch the 8-byte parquet footer and  \
+                 another to fetch the metadata length encoded in the footer.",
+                DataType::Boolean,


Why is this a boolean?

You are right -- it should be u64 -- fixed in e5e3be0

tustvold

I'm really struggling to review this PR as I'm honestly lost trying to understand the design of the config system...

We used to have just RuntimeConfig/ RuntimeEnv for process global settings, SessionConfig/SessionContext for session local settings i.e. planning, and TaskContext for physical plan execution settings

I'm not entirely clear on how ConfigOptions fits into this, what scope it has, etc... It appears to have been introduced by @andygrove in #2754 with the intention articulated on #2756 to use it for SessionContext

Plumbing this down into the physical plans therefore feels a bit like it is coupling together parts of the system that shouldn't be coupled together? I would have expected the physical operators to only be exposed to TaskContext, with SessionContext solely used for planning.

I'm not saying the approach in this PR is wrong, but I don't know what correct looks like 😅

tustvold · 2022-10-24T02:20:48Z

datafusion/core/src/execution/options.rs

        }
    }
 }

 impl<'a> ParquetReadOptions<'a> {
-    /// Specify parquet_pruning
-    pub fn parquet_pruning(mut self, parquet_pruning: bool) -> Self {


Doesn't this represent a functionality loss, previously you could configure these options on per-datasource basis. Now they are located on ConfigOptions which is session global?

I would say "kind of" -- previously you could configure the options on a per-datasource basis but depending on exactly what codepath you used and what other options were set your settings might or might not get overridden.

…arquet

alamb · 2022-10-24T21:17:40Z

I'm really struggling to review this PR as I'm honestly lost trying to understand the design of the config system...

Yes, I agree it is very confusing.

Would you find it less confusing if the physical planner got a SessionConfig rather than ConfigOptions?

Plumbing this down into the physical plans therefore feels a bit like it is coupling together parts of the system that shouldn't be coupled together? I would have expected the physical operators to only be exposed to TaskContext, with SessionContext solely used for planning.

I think the SessionContext has a SessionState which has a SessionConfig which has a ConfigOptions 🤯

TaskContext has TaskProperties which is either a SessionConfig or some k=v pairs

So in that view of things your expectation "expected the physical operators to only be exposed to TaskContext, with SessionContext solely used for planning." does hold

In my mind ConfigOptions and SessionConfig serve the same purpose and eventually they will be unified (using the implementation of ConfigOptions.

alamb · 2022-10-24T21:19:36Z

Here is my plan: I will write up a ticket explaining the current state of affairs in terms of config (and I will make a diagram) and we can use that to come to consensus on where we want to head.

Given the current mess of parquet options (has 3 other places to place config values in addition to the SessonConfig / ConfigOptions I think this PR is an improvement anyways.

We'll see if @andygrove has any thoughts

alamb · 2022-10-25T20:06:38Z

Marking as a draft until I get a writeup and some consensus

alamb · 2022-11-23T22:44:43Z

Here is a writeup of a way to improve the configuration situation somewhat #4349

thinkharderdev · 2022-11-25T13:05:37Z

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),


Do we want to default this something? Back in the day we would default to reading the last 64k to try and capture the entire footer in a single read which seems like a sensible default (especially for object storage where the cost of an additional read is very expensive relative to reading a bit more data in the first read)

64K seems reasonable to me

Upon more thought I would like to propose we do the "consolidate config code" and "change default value" in separate PRs (which I think will be easier to see / review once the configuration is consolidated)

alamb · 2022-11-26T13:49:29Z

FWIW My plan for this PR is:

To leave any the current places to configure parquet settings per ParquetExec as overrrides (Option<val>)
Expose remaining options via ConfigOptions (and make them visible, etc)

alamb · 2022-11-29T22:28:38Z

Updated version in #4427

alamb added the api change Changes the API exposed to users of the crate label Oct 18, 2022

alamb marked this pull request as draft October 18, 2022 17:30

alamb mentioned this pull request Oct 18, 2022

Consolidate SessionConfig and ConfigOptions #3887

Open

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

alamb force-pushed the alamb/consolidate_parquet branch from 23a6614 to 2addab3 Compare October 20, 2022 16:28

Consolidate remaining parquet config options into ConfigOptions

8180c3a

alamb force-pushed the alamb/consolidate_parquet branch from 2addab3 to 8180c3a Compare October 20, 2022 16:42

alamb commented Oct 20, 2022

View reviewed changes

alamb marked this pull request as ready for review October 20, 2022 16:53

alamb mentioned this pull request Oct 20, 2022

Make ConfigOptions easier to work with #3909

Closed

alamb requested review from tustvold, thinkharderdev and liukun4515 October 22, 2022 11:34

tustvold reviewed Oct 24, 2022

View reviewed changes

alamb added 2 commits October 24, 2022 17:07

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

1f2067f

…arquet

fix: type should be u64

e5e3be0

alamb marked this pull request as draft October 25, 2022 20:06

alamb mentioned this pull request Nov 2, 2022

Support SET command #4067

Closed

alamb mentioned this pull request Nov 23, 2022

DataFusion Configuration Consolidation #4349

Closed

6 tasks

thinkharderdev reviewed Nov 25, 2022

View reviewed changes

alamb mentioned this pull request Nov 29, 2022

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

Merged

alamb closed this Nov 29, 2022

alamb mentioned this pull request Dec 1, 2022

Default parquet reader to reading 64K footer #4459

Open

alamb deleted the alamb/consolidate_parquet branch August 8, 2023 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate remaining parquet config options into ConfigOptions #3885

Consolidate remaining parquet config options into ConfigOptions #3885

alamb commented Oct 18, 2022 •

edited

Loading

alamb commented Oct 18, 2022

alamb Oct 20, 2022

alamb Oct 20, 2022

alamb Oct 20, 2022

alamb Oct 20, 2022

alamb Oct 20, 2022

tustvold Oct 24, 2022

alamb Oct 24, 2022

tustvold left a comment •

edited

Loading

tustvold Oct 24, 2022

alamb Oct 25, 2022

alamb commented Oct 24, 2022

alamb commented Oct 24, 2022

alamb commented Oct 25, 2022

alamb commented Nov 23, 2022

thinkharderdev Nov 25, 2022

alamb Nov 25, 2022

alamb Nov 29, 2022

alamb commented Nov 26, 2022

alamb commented Nov 29, 2022

Consolidate remaining parquet config options into ConfigOptions #3885

Consolidate remaining parquet config options into ConfigOptions #3885

Conversation

alamb commented Oct 18, 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?

alamb commented Oct 18, 2022

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

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Oct 24, 2022

alamb commented Oct 24, 2022

alamb commented Oct 25, 2022

alamb commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 26, 2022

alamb commented Nov 29, 2022

alamb commented Oct 18, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading