Don't share ConfigOptions (#3886) #4712

tustvold · 2022-12-22T12:16:43Z

Which issue does this PR close?

Closes #3886
Closes #3909
Relates to #4349
Relates to #4617

Rationale for this change

Having shared mutable state makes reasoning about mutation difficult (#4617), the locking is verbose and potentially error prone (#3886),

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

tustvold · 2022-12-22T12:18:15Z

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

@@ -302,6 +287,8 @@ impl ExecutionPlan for ParquetExec {
 })
 })?;

+ let config_options = ctx.session_config().config_options();


We need to fetch this at execution time, in order that datafusion-proto can still deserialize ParquetExec without a SessionState. Longer term as we strip out the overrides this will make more sense anyway so 🤷

I think it is reasonable to look at the session configuration while executing 🤷

It certainly seems better than the current state of master where the config options (attached to session state) are read via interior mutability

tustvold · 2022-12-22T12:19:25Z

datafusion/proto/proto/datafusion.proto

@@ -90,7 +90,8 @@ message CsvFormat {
 }


I recommend viewing this with whitespace disabled

tustvold · 2022-12-22T12:21:31Z

datafusion/proto/src/logical_plan/mod.rs

@@ -353,12 +353,9 @@ impl AsLogicalPlan for LogicalPlanNode {
 self
 ))
 })? {
- &FileFormatType::Parquet(protobuf::ParquetFormat {


The plumbing for this override was actually incorrect, it would convert false -> None, the other overrides aren't present, and we plan to remove this override mechanism as part of #4349 so I just opted to remove it

I agree serializing the same config options multiple times (once in the main session context and then once again as part of the file format) is undesirable for many reasons

tustvold · 2022-12-22T17:39:43Z

parquet-test-utils/src/lib.rs

+impl ParquetScanOptions {
+ /// Returns a [`SessionConfig`] with the given options
+ pub fn config(&self) -> SessionConfig {
+ SessionConfig::new()


I debated simply removing ParquetScanOptions in favour of SessionConfig but figured this PR was large enough as it was

Thank you. I agree this PR is already large. I also think the ParquetScanOptions predated the config options.

I think removing the ParquetScanOptions as a follow on PR is a good idea 👍

alamb

👨‍🍳 👌

This looks really good @tustvold -- thank you for helping sort out the configuration situation

alamb · 2022-12-22T20:25:48Z

datafusion-examples/examples/flight_server.rs

- Pin<Box<dyn Stream<Item = Result<ActionType, Status>> + Send + Sync + 'static>>;
- type DoExchangeStream =
- Pin<Box<dyn Stream<Item = Result<FlightData, Status>> + Send + Sync + 'static>>;
+ type HandshakeStream = BoxStream<'static, Result<HandshakeResponse, Status>>;


I forgot this was here -- I have to give this example love to give this after my work to make arrow-flight easier to use

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

alamb · 2022-12-22T20:33:06Z

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

@@ -85,13 +84,9 @@ impl ParquetFormat {
 }

 /// Return true if pruning is enabled
- pub fn enable_pruning(&self) -> bool {
+ pub fn enable_pruning(&self, config_options: &ConfigOptions) -> bool {


alamb · 2022-12-22T20:34:51Z

datafusion/core/src/execution/context.rs

@@ -1173,7 +1166,7 @@ pub struct SessionConfig {
 /// due to `resolve_table_ref` which passes back references)
 default_schema: String,
 /// Configuration options
- pub config_options: Arc<RwLock<ConfigOptions>>,
+ config_options: ConfigOptions,


alamb · 2022-12-22T20:38:57Z

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

@@ -302,6 +287,8 @@ impl ExecutionPlan for ParquetExec {
 })
 })?;

+ let config_options = ctx.session_config().config_options();


I think it is reasonable to look at the session configuration while executing 🤷

It certainly seems better than the current state of master where the config options (attached to session state) are read via interior mutability

alamb · 2022-12-22T20:49:26Z

datafusion/proto/proto/datafusion.proto

- CurrentDate=70;
- CurrentTime=71;
- Uuid=72;
+ Abs = 0;


whitespace!

alamb · 2022-12-22T20:49:49Z

datafusion/proto/proto/datafusion.proto

@@ -1132,6 +1133,9 @@ message ScanLimit {
 }

 message FileScanExecConf {
+ // Was repeated ConfigOption options = 10;
+ reserved 10;


alamb · 2022-12-22T20:50:56Z

datafusion/proto/src/logical_plan/mod.rs

@@ -353,12 +353,9 @@ impl AsLogicalPlan for LogicalPlanNode {
 self
 ))
 })? {
- &FileFormatType::Parquet(protobuf::ParquetFormat {


I agree serializing the same config options multiple times (once in the main session context and then once again as part of the file format) is undesirable for many reasons

alamb · 2022-12-22T20:51:21Z

parquet-test-utils/src/lib.rs

+impl ParquetScanOptions {
+ /// Returns a [`SessionConfig`] with the given options
+ pub fn config(&self) -> SessionConfig {
+ SessionConfig::new()


Thank you. I agree this PR is already large. I also think the ParquetScanOptions predated the config options.

I think removing the ParquetScanOptions as a follow on PR is a good idea 👍

Co-authored-by: Andrew Lamb <[email protected]>

ursabot · 2022-12-23T10:41:35Z

Benchmark runs are scheduled for baseline = afb1ae2 and contender = 07f4980. 07f4980 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

github-actions bot added the core Core datafusion crate label Dec 22, 2022

tustvold commented Dec 22, 2022

View reviewed changes

tustvold force-pushed the no-shared-config-options branch 2 times, most recently from b650b86 to 3327d11 Compare December 22, 2022 12:31

Don't share ConfigOptions (apache#3886)

00a9b28

tustvold force-pushed the no-shared-config-options branch from 3327d11 to 00a9b28 Compare December 22, 2022 12:43

More tweaks

ecdd866

tustvold added the api change Changes the API exposed to users of the crate label Dec 22, 2022

tustvold added 2 commits December 22, 2022 17:35

Fix TestParquetFile

ea28847

Clippy

88e57ba

tustvold commented Dec 22, 2022

View reviewed changes

Fix doctest

477fd7d

alamb reviewed Dec 22, 2022

View reviewed changes

alamb approved these changes Dec 22, 2022

View reviewed changes

Apply suggestions from code review

e3a8232

Co-authored-by: Andrew Lamb <[email protected]>

tustvold merged commit 07f4980 into apache:master Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't share ConfigOptions (#3886) #4712

Don't share ConfigOptions (#3886) #4712

tustvold commented Dec 22, 2022 •

edited by alamb

Loading

tustvold Dec 22, 2022

alamb Dec 22, 2022

tustvold Dec 22, 2022

tustvold Dec 22, 2022

alamb Dec 22, 2022

tustvold Dec 22, 2022

alamb Dec 22, 2022

alamb left a comment

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

alamb Dec 22, 2022

ursabot commented Dec 23, 2022

Don't share ConfigOptions (#3886) #4712

Don't share ConfigOptions (#3886) #4712

Conversation

tustvold commented Dec 22, 2022 • edited by alamb 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

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

Choose a reason for hiding this comment

ursabot commented Dec 23, 2022

tustvold commented Dec 22, 2022 •

edited by alamb

Loading