feat: allow passing a SessionState into a OptimizeBuilder
#3802
Conversation
Signed-off-by: Abhi Agarwal <[email protected]>
abhi-airspace-intelligence
requested review from
hntd187,
roeap and
rtyler
as code owners
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
| /// A session state accompanying a given input plan, containing e.g. registered object stores | ||
| pub fn with_input_session_state(mut self, state: SessionState) -> Self { | ||
| self.state = Some(state); | ||
| self | ||
| } |
There was a problem hiding this comment.
Note that the name of this function is inconsistent across the various builder APIs, happy to open a follow-up PR to rename everything to either with_input_session_state or with_session_state and deprecate the other.
abhi-airspace-intelligence
changed the title
SessionState into a OptimizeBuilderSessionState into a OptimizeBuilder
|
@ion-elgreco I'll do that in a follow-up PR to minimize blast radius! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3802 +/- ##
==========================================
- Coverage 76.06% 76.06% -0.01%
==========================================
Files 145 145
Lines 45273 45276 +3
Branches 45273 45276 +3
==========================================
- Hits 34439 34438 -1
- Misses 9144 9147 +3
- Partials 1690 1691 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// Optional [SessionConfig] for users that want more control over the Datafusion execution | ||
| session_config: Option<SessionConfig>, | ||
| /// Datafusion session state relevant for executing the input plan | ||
| state: Option<SessionState>, |
There was a problem hiding this comment.
Haven't looked at all the APIs, but would we get away with tracking something like Arc<dyn Session> here?
There was a problem hiding this comment.
Nice, looks like that's preferred by Datafusion to pass this instead. https://docs.rs/datafusion-session/50.1.0/datafusion_session/session/trait.Session.html#migration-from-sessionstate. Let me give it a shot
There was a problem hiding this comment.
@roeap I will resolve this in a follow-up PR, since I'm unifying all the interfaces anyways
|
@abhi-airspace-intelligence - blocked the PR not b/c its a pre-requisite, just wanted to ask. If not feasible we can likely merge as is. |
3 participants
Description
This allows passing a
SessionStateinto anOptimizeBuilder, mirroring the other operation APIs. Very useful if running concurrent delta table operations in a kubernetes pod, as you can limit Datafusion to use a certain amount of ram between all shared contexts to prevent OOMsRelated Issue(s)
Closes #3797, effectively reverts/supersedes #3751