Skip to content

Add SessionStateBuilder and extract out the registration of defaults #11403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 10 commits into from
Jul 14, 2024
Merged

Add SessionStateBuilder and extract out the registration of defaults #11403

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fab06da
Create a SessionStateBuilder and use it for creating anything but a b…
Omega359 Jul 10, 2024
de39082
Updated new_from_existing to take a reference to the existing Session…
Omega359 Jul 10, 2024
7d1bc09
Minor documentation update.
Omega359 Jul 10, 2024
490214d
SessionStateDefaults improvements.
Omega359 Jul 10, 2024
93a3a7d
Merge branch 'apache:main' into feature/11320_session_state_builder
Omega359 Jul 10, 2024
e3fad6d
Reworked how SessionStateBuilder works from PR feedback.
Omega359 Jul 12, 2024
8a579cf
Bug fix for missing array_expressions cfg feature.
Omega359 Jul 12, 2024
eb64e2f
Merge remote-tracking branch 'refs/remotes/upstream/main' into featur…
Omega359 Jul 14, 2024
71decf6
Review feedback updates + doc fixes for SessionStateDefaults
Omega359 Jul 14, 2024
380ce98
Cargo fmt update.
Omega359 Jul 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Minor documentation update.
  • Loading branch information
@Omega359
Omega359 committed Jul 10, 2024
commit 7d1bc09a1c20d8a2f7560032b5b29624b4fae164
4 changes: 2 additions & 2 deletions datafusion/core/src/execution/session_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,8 +886,8 @@ impl SessionStateBuilder {
}

/// Returns a new [SessionStateBuilder] based on an existing [SessionState]
/// The session id for the new builder will be reset to a unique value, all
/// other fields will be set to what is set in the provided session state
/// The session id for the new builder will be set to a unique value; all
/// other fields will be cloned from what is set in the provided session state
pub fn new_from_existing(existing: &SessionState) -> Self {
Copy link
Contributor

@alamb alamb Jul 12, 2024

Choose a reason for hiding this comment

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

Since this clones SessionState anyways, I think it would make more sense for this to take the argument by value

Suggested change
pub fn new_from_existing(existing: &SessionState) -> Self {
pub fn new_from_existing(existing: SessionState) -> Self {

The rationale being there if the caller already has an owned SessionState they can use that and avoid a clone. This API basically requires them to do a clone

Copy link
Contributor

@alamb alamb Jul 12, 2024

Choose a reason for hiding this comment

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

In addition to this function, another nice to have UX feature would be a From impl, so users could do

let state: SessionState = get_existing_state();
let state = SessionStateBuilder::from(state)
  .with_optimizer_pass(Arc::new(MyNewOptimizer{}))
  .build()

let session_id = Uuid::new_v4().to_string();

Expand Down