-
Couldn't load subscription status.
- Fork 537
chore: unify inconsistent SessionState in datafusion operations
#3816
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.
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
chore: unify inconsistent SessionState in datafusion operations
#3816
Conversation
|
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. |
SessionState in datafusion operations
5e3380b to
841646f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3816 +/- ##
==========================================
- Coverage 74.40% 74.32% -0.09%
==========================================
Files 147 147
Lines 39544 39585 +41
Branches 39544 39585 +41
==========================================
- Hits 29423 29421 -2
- Misses 8734 8775 +41
- Partials 1387 1389 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Marking this as draft until the conflicts are resolved 🙈 |
|
Should be good to go. @roeap do you have a preference on what the var should be for datafusion state? In some places it's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great @abhi-airspace-intelligence.
As for the name i think session is the way to go, as this is the traits name. state would be second, since we often used the implementation SessionState.
We also do have some fixable lints (unused imports). Granted these are hard to track in our codebase right now, since there are SOOOO many deprecations. but maybe try to fix these :). - i.e. whatever makes check fail.
I'll remove the linked issue, since I believe we should also centralise setting up the session in a dedicated function o.a. since we still need to create session in several places. Important step towards consolidation though!!
|
@roeap I will fix the lints and update the name tomorrow! |
Signed-off-by: Abhi Agarwal <[email protected]>
Signed-off-by: Abhi Agarwal <[email protected]>
Signed-off-by: Abhi Agarwal <[email protected]>
Signed-off-by: Abhi Agarwal <[email protected]>
Signed-off-by: Abhi Agarwal <[email protected]>
e029662 to
4adcecf
Compare
Signed-off-by: Abhi Agarwal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
Unifies the SessionState management for the various DeltaOps by moving it to use the
dyn Sessioninterface, downcasting toSessionContextwhen relevant.Documentation
Sessiontrait in datafusion: https://docs.rs/datafusion/50.0.0/datafusion/catalog/trait.Session.html