Skip to content

Conversation

@waynexia
Copy link
Member

Signed-off-by: Ruihang Xia waynestxia@gmail.com

Which issue does this PR close?

NAN

Rationale for this change

Change the API for register_* (like register_udf) in SessionContext. Relax the share requirement from &mut self to &self. Because the actual state is wrapped in a RwLock (state: Arc<RwLock<SessionState>>), it's unnecessary to require &mut self for SessionContext itself.

What changes are included in this PR?

Relax the share requirement from &mut self to &self.

Are these changes tested?

No need I suppose.

Are there any user-facing changes?

Yes, this is API change.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@github-actions github-actions bot added the core Core DataFusion crate label Dec 14, 2022
@tustvold
Copy link
Contributor

I wonder if perhaps this is a sign that we don't really need these locks 🤔 will have a play around

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@waynexia
Copy link
Member Author

I wonder if perhaps this is a sign that we don't really need these locks 🤔 will have a play around

IMO that lock cannot be avoided. If we don't put it over SessionState then we need to either put it in the SessionState or change all the methods to require &mut self, which is likely to be put inside another lock to achieve the "interior mutability" 🥲

@alamb
Copy link
Contributor

alamb commented Dec 14, 2022

BTW I think @tustvold has some larger ideas of how to simplify the configuration system -- see #4617 and related tickets

I think it is fine to merge this PR , even though there is likely going to be some churn in this API anyways

@alamb alamb merged commit da0de9d into apache:master Dec 14, 2022
@ursabot
Copy link

ursabot commented Dec 14, 2022

Benchmark runs are scheduled for baseline = 4542031 and contender = da0de9d. da0de9d 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

@waynexia waynexia deleted the relax-session-ctx branch December 15, 2022 02:55
@waynexia
Copy link
Member Author

Thanks for that info!

@mingmwang
Copy link
Contributor

I remember when I refactor the original ExecutionContext to SessionContext, I also thought the signature of the &mut self can be relaxed, the reason I didn't make this change in those PRs is just to keep them small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants