Structify ConfigOptions (#4517) #4771

tustvold · 2022-12-29T15:37:57Z

Which issue does this PR close?

Closes #4517
Closes #3500
Closes #4752
Relates to #3887

Rationale for this change

The current approach is:

Not type-safe
Not documented by rustdoc
Not understood by code completion
Expensive to clone
Defaulting logic is not centralised
Inconsistently serializes to TaskContext

What changes are included in this PR?

This reworks ConfigOptions to instead be a struct, using a declarative macro to auto-generate serialization logic

Are these changes tested?

Are there any user-facing changes?

Yes

datafusion/core/src/physical_plan/coalesce_batches.rs

tustvold · 2022-12-29T15:38:47Z

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

    fn enable_page_index(&self, config_options: &ConfigOptions) -> bool {
        self.enable_page_index
-            .or_else(|| config_options.get_bool(OPT_PARQUET_ENABLE_PAGE_INDEX))
-            // default to false


We no longer have implicit defaults all over the place 😄

tustvold · 2022-12-29T15:39:57Z

datafusion/core/src/config.rs

-            .finish()
+/// A type-safe container for [`ConfigExtension`]
+#[derive(Debug, Default, Clone)]
+pub struct Extensions(BTreeMap<&'static str, ExtensionBox>);


This formulation is specifically designed to be able to replace the AnyMap currently on SessionConfig. As an added bonus it no longer requires types to be interior-mutable

tustvold · 2022-12-29T15:40:51Z

datafusion/core/src/config.rs

+        pub round_robin_repartition: bool, default = true
+
+        /// Parquet options
+        pub parquet: ParquetOptions, default = Default::default()


This nesting is a source of non-trivial additional complexity, but I made it work so at least we have support for this in the future.

…options

tustvold · 2022-12-29T18:50:50Z

datafusion/core/src/execution/context.rs

    }
 }

+impl From<ConfigOptions> for SessionConfig {


This lays the ground-word for eventually removing SessionConfig altogether #3887

tustvold · 2022-12-29T18:51:31Z

datafusion/core/src/execution/context.rs

-        self.config_options().get(variable)
+        // TOOD: Move ConfigOptions into common crate
+        match variable {
+            "datafusion.execution.time_zone" => self


This is a nasty hack, I hope to move ConfigOptions into core in a subsequent PR so that it can be used directly instead of via ContextProvider

tustvold · 2022-12-29T20:00:33Z

datafusion/core/tests/sqllogictests/test_files/information_schema.slt

 ----
 datafusion.catalog.create_default_catalog_and_schema true
+datafusion.catalog.format NULL
+datafusion.catalog.has_header false


This option was missing from the definitions

tustvold · 2022-12-29T20:01:13Z

datafusion/core/tests/sqllogictests/test_files/information_schema.slt

+datafusion.catalog.has_header false
 datafusion.catalog.information_schema true
 datafusion.catalog.location NULL
-datafusion.catalog.type NULL


type is a reserved keyword in rust, and the name was pretty overloaded anyway, so I opted to rename this to format. Amusingly this was already what the local variables were named as

…options

tustvold · 2022-12-30T11:18:22Z

datafusion/core/tests/fifo.rs

-        let config = SessionConfig::new()
-            .with_batch_size(TEST_BATCH_SIZE)
-            .set_u64(
-                "datafusion.execution.coalesce_target_batch_size",


This was missed in #4757 the new config returns an error for invalid keys and so caught this

tustvold · 2022-12-30T11:19:21Z

datafusion/core/src/execution/context.rs

These two were added in #4757. They're no longer necessary and so I just opted to remove them

tustvold · 2022-12-30T11:21:41Z

datafusion/core/src/execution/context.rs

-        };
+        let mut config = ConfigOptions::new();
+        for (k, v) in task_props {
+            let _ = config.set(&k, &v);


This shows deserializing from a HashMap, as an added bonus this will now work for all keys and not just a specific subset

Longer-term we probably want to change this signature to accept ConfigOptions directly

related to #4758

tustvold · 2022-12-30T11:22:50Z

datafusion/core/src/config.rs

-        self.set(key, ScalarValue::Utf8(Some(value.into())))
-    }
+    /// Returns the [`ConfigEntry`] stored within this [`ConfigOptions`]
+    pub fn entries(&self) -> Vec<ConfigEntry> {


This is the mechanism by which ConfigOptions can be both serialized and documentation generated

alamb

I really like how the options are now typed / type checked at compile time rather than runtime.

cc @yahoNanJing @andygrove and @thinkharderdev

alamb · 2022-12-30T15:55:07Z

benchmarks/src/bin/h2o.rs

    let path = opt.path.to_str().unwrap();
-    let config = SessionConfig::from_env().with_batch_size(65535);
+    let mut config = ConfigOptions::from_env()?;
+    config.built_in.execution.batch_size = 65535;


this is kind of cool that it is a struct format that is checked by the compiler

datafusion/core/src/physical_optimizer/enforcement.rs

alamb

I love it

The created structure works well in my editor.

Thank you @tustvold . This is the one missing

It think we should leave this open at least a full work day after approval (aka don't merge until next Monday)

alamb · 2022-12-30T16:04:55Z

datafusion/core/src/execution/context.rs

    default_schema: String,
    /// Configuration options
-    config_options: ConfigOptions,
+    options: ConfigOptions,


👎 for the change to the more overloaded options name, even though I realize it is more concise

Yeah, fustfmt was being obtuse and formatting things over 7 lines and this made it happy. SessionConfig isn't long for this world so I think it is fine

alamb · 2022-12-30T16:06:25Z

datafusion/core/src/execution/context.rs

+    /// replaced by [`config_options`].
+    ///
+    /// [`config_options`]: SessionContext::config_option
+    pub fn to_props(&self) -> HashMap<String, String> {


This may be related to #4758 from @yahoNanJing

alamb · 2022-12-30T16:07:11Z

datafusion/core/src/execution/context.rs

-        };
+        let mut config = ConfigOptions::new();
+        for (k, v) in task_props {
+            let _ = config.set(&k, &v);


related to #4758

alamb · 2022-12-30T16:08:20Z

docs/source/user-guide/configs.md

-| datafusion.optimizer.repartition_windows                  | Boolean | true    | Should DataFusion repartition data using the partitions keys to execute window functions in parallel using the provided `target_partitions` level                                                                                                                                                                                             |
-| datafusion.optimizer.skip_failed_rules                    | Boolean | true    | When set to true, the logical plan optimizer will produce warning messages if any optimization rules produce errors and then proceed to the next rule. When set to false, any rules that produce errors will cause the query to fail.                                                                                                         |
-| datafusion.optimizer.top_down_join_key_reordering         | Boolean | true    | When set to true, the physical plan optimizer will run a top down process to reorder the join keys. Defaults to true                                                                                                                                                                                                                          |
+| key                                                       | default | description                                                                                                                                                                                                                                                                                                |


The major difference here seems to be the type column is no longer present. Was that intended?

Yeah, the fields are now Rust typed not datatype. I could include this, but it seemed unnecessary?

It is probably clear from the names if something is boolean or numeric. I don't feel strongly

alamb · 2022-12-30T16:09:25Z

datafusion/core/src/config.rs

+use std::collections::BTreeMap;
+use std::fmt::Display;
+
+macro_rules! config_namespace {


👍

I think some introduction doc comments here would help -- specifically some examples of what this macro is creating would be nice

tustvold · 2023-01-02T17:08:01Z

I plan to merge this tomorrow morning (GMT) unless anyone needs more time to review

ursabot · 2023-01-03T08:41:45Z

Benchmark runs are scheduled for baseline = 6c81c10 and contender = 21169b9. 21169b9 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

Structify ConfigOptions (apache#4517)

4498533

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

tustvold commented Dec 29, 2022

View reviewed changes

datafusion/core/src/physical_plan/coalesce_batches.rs Outdated Show resolved Hide resolved

tustvold commented Dec 29, 2022

View reviewed changes

tustvold added 7 commits December 29, 2022 17:32

Simplify

54c2e95

Fix environment variables

b5a2241

Merge remote-tracking branch 'upstream/master' into structify-config-…

955a573

…options

Tweaks

c5bae84

Update datafusion-cli

af2a338

Format

e826dfa

Misc cleanup

f6ef943

tustvold commented Dec 29, 2022

View reviewed changes

tustvold marked this pull request as ready for review December 29, 2022 18:52

Further fixes

e63237d

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Dec 29, 2022

tustvold commented Dec 29, 2022

View reviewed changes

tustvold requested review from alamb and andygrove December 29, 2022 20:01

tustvold added 2 commits December 29, 2022 20:04

Format

bafec8b

Update config.md

f316e0f

This was referenced Dec 29, 2022

Move subquery alias assignment onto rules #4767

Merged

Avoid boxing config keys #4762

Closed

Merge remote-tracking branch 'upstream/master' into structify-config-…

d3a542e

…options

tustvold commented Dec 30, 2022

View reviewed changes

tustvold mentioned this pull request Dec 30, 2022

Provide a constructor for the ConfigOptions with a string hash map #4758

Closed

tustvold commented Dec 30, 2022

View reviewed changes

alamb reviewed Dec 30, 2022

View reviewed changes

alamb approved these changes Dec 30, 2022

View reviewed changes

tustvold added 2 commits December 30, 2022 18:13

Flatten configuration

ee2255f

Document macro

bd68058

tustvold merged commit 21169b9 into apache:master Jan 3, 2023

alamb mentioned this pull request Jan 23, 2023

minor: Fix docs for create_default_catalog_and_schema #5032

Merged

Structify ConfigOptions (#4517) #4771

Structify ConfigOptions (#4517) #4771

Uh oh!

Conversation

tustvold commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold commented Jan 2, 2023

Uh oh!

ursabot commented Jan 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tustvold commented Dec 29, 2022 •

edited

Loading

tustvold Dec 30, 2022 •

edited

Loading