DataFrame owned SessionState #4633

tustvold · 2022-12-14T22:37:24Z

Which issue does this PR close?

Part of #4617

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb · 2022-12-15T19:23:46Z

🤔 there appears to be some sort of deadlock in the tests

https://github.com/apache/arrow-datafusion/actions/runs/3702235488/jobs/6272289305


test sql::select::select_values_list ... ok
test sql::select::test_prepare_statement ... ok
test sql::select::unprojected_filter ... ok
test sql::select::use_between_expression_in_select_query ... ok
test sql::order::sort_with_lots_of_repetition_values ... ok
test sql::set_variable::set_bool_variable_bad_value ... ok
test sql::set_variable::set_bool_variable has been running for over 60 seconds
test sql::set_variable::set_time_zone has been running for over 60 seconds
Error: The operation was canceled.

alamb

Thank you @tustvold the idea looks great to me, though I think we need to solve the CI deadlock

tustvold · 2022-12-15T19:32:11Z

Lol, even more reason to remove these locks #4617

tustvold · 2022-12-15T19:32:53Z

datafusion/core/src/execution/context.rs

@@ -363,7 +363,8 @@ impl SessionContext {
            LogicalPlan::SetVariable(SetVariable {
                variable, value, ..
            }) => {
-                let config_options = &self.state.write().config.config_options;


I'm actually somewhat surprised this was compiling, there must be some magic going on to extend the lifetime of the temporary lock guard, which is wild

tustvold · 2022-12-16T14:44:36Z

datafusion/core/src/dataframe.rs

-            // original state after it has been cloned, they will not be picked up by the
-            // clone but that is okay, as it is equivalent to postponing the state update
-            // by keeping the lock until the end of the function scope.
-            state.clone()


In practice the lock meant that we were still cloning SessionState fairly frequently, better to just be explicit about it and optimise from there

tustvold · 2022-12-16T14:45:19Z

datafusion/core/src/dataframe.rs


-        state_cloned.create_physical_plan(&self.plan).await
+    /// Temporary pending #4626


This is temporary pending moving the physical plan lowering onto DataFrame from SessionState which is tracked by #4626 and #4629

tustvold · 2022-12-16T15:01:14Z

datafusion/core/src/dataframe.rs

-        state_cloned.create_physical_plan(&self.plan).await
+    /// Temporary pending #4626
+    async fn create_physical_plan_impl(&mut self) -> Result<Arc<dyn ExecutionPlan>> {
+        self.session_state.execution_props.start_execution();


This is nice because we are no longer mutating state on the actual SessionContext

alamb

I like it -- thank you @tustvold

There is likely a change of behavior here:

Specifically prior to this PR if you made a dataframe and then modified the SessionContext the dataframe might see some of those changes.

However I think the old behavior would be quite surprising and so I like this change a lot

alamb · 2022-12-16T19:37:12Z

datafusion/core/src/dataframe.rs

    plan: LogicalPlan,
 }

 impl DataFrame {
    /// Create a new Table based on an existing logical plan
-    pub fn new(session_state: Arc<RwLock<SessionState>>, plan: LogicalPlan) -> Self {
+    pub fn new(session_state: SessionState, plan: LogicalPlan) -> Self {


alamb · 2022-12-16T19:41:09Z

datafusion/core/src/dataframe.rs

@@ -620,28 +604,25 @@ impl DataFrame {
    }

    /// Write a `DataFrame` to a CSV file.
-    pub async fn write_csv(self, path: &str) -> Result<()> {
-        let state = self.session_state.read().clone();


yeah all the explicit cloning is definitely a warning sign

ursabot · 2022-12-17T10:32:08Z

Benchmark runs are scheduled for baseline = 414487c and contender = 42b3a6c. 42b3a6c 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

mingmwang · 2022-12-20T02:32:14Z

@tustvold @alamb

I'm a little surprised by this change. In my opinion, SessionState should be the internal state of a SessionContext, it can be borrowed/shared, making SessionState owned by the DataFrame will break the purpose that it is the state of the session. We should not have many SessionStates for a single Session in the system.

tustvold · 2022-12-20T09:08:36Z

The idea is that DataFrame contains a snapshot of SessionContext, i.e. a clone of SessionState? This will allow moving planning and execution off SessionContext to both resolve the current circular dependency, and also make the state consistent across planning and execution

alamb · 2022-12-20T10:26:11Z

@mingmwang I think the key proposal by @tustvold is that once a query is planned, it has only a read-only copy of the SessionState rather than mutable access to the current SessionState

This has several seemingly nice properties, such as a configuration value changes that are made after a query is planned, will not affect the plan (it will only affect subsequently planned queries). Do you agree this sounds like reasonable behavior? Update: I didn't see #4617 (comment) so it sounds like this is reasonable from your perspective

Do you know of any usecases where running a query needs to modify the session context?

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

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

DataFrame owned SessionState (apache#4617)

215f8db

tustvold force-pushed the dataframe-arc-rwlock branch from d2091d9 to 215f8db Compare December 15, 2022 08:10

tustvold marked this pull request as ready for review December 15, 2022 08:43

tustvold requested a review from alamb December 15, 2022 11:51

alamb reviewed Dec 15, 2022

View reviewed changes

Fix deadlock

5a9ca2d

tustvold commented Dec 15, 2022

View reviewed changes

tustvold added 2 commits December 16, 2022 14:31

Merge remote-tracking branch 'upstream/master' into dataframe-arc-rwlock

d6766f8

Fix execution time

54f855a

tustvold commented Dec 16, 2022

View reviewed changes

alamb approved these changes Dec 16, 2022

View reviewed changes

tustvold merged commit 42b3a6c into apache:master Dec 17, 2022

mingmwang mentioned this pull request Dec 20, 2022

Configuration Mutation Isolation #4617

Closed

andygrove mentioned this pull request Apr 10, 2024

Fix large futures causing stack overflows #10033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataFrame owned SessionState #4633

DataFrame owned SessionState #4633

tustvold commented Dec 14, 2022 •

edited

Loading

alamb commented Dec 15, 2022

alamb left a comment

tustvold commented Dec 15, 2022

tustvold Dec 15, 2022

tustvold Dec 16, 2022

tustvold Dec 16, 2022

tustvold Dec 16, 2022

alamb left a comment

alamb Dec 16, 2022

alamb Dec 16, 2022

ursabot commented Dec 17, 2022

mingmwang commented Dec 20, 2022

tustvold commented Dec 20, 2022 •

edited

Loading

alamb commented Dec 20, 2022 •

edited

Loading


		state_cloned.create_physical_plan(&self.plan).await
		/// Temporary pending #4626

DataFrame owned SessionState #4633

DataFrame owned SessionState #4633

Conversation

tustvold commented Dec 14, 2022 • edited 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?

alamb commented Dec 15, 2022

alamb left a comment

Choose a reason for hiding this comment

tustvold commented Dec 15, 2022

tustvold Dec 15, 2022

Choose a reason for hiding this comment

tustvold Dec 16, 2022

Choose a reason for hiding this comment

tustvold Dec 16, 2022

Choose a reason for hiding this comment

tustvold Dec 16, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Dec 16, 2022

Choose a reason for hiding this comment

alamb Dec 16, 2022

Choose a reason for hiding this comment

ursabot commented Dec 17, 2022

mingmwang commented Dec 20, 2022

tustvold commented Dec 20, 2022 • edited Loading

alamb commented Dec 20, 2022 • edited Loading

tustvold commented Dec 14, 2022 •

edited

Loading

tustvold commented Dec 20, 2022 •

edited

Loading

alamb commented Dec 20, 2022 •

edited

Loading