feat(substrait): replace SessionContext with a trait #13343
Conversation
| pub(crate) fn schema_for_ref( | ||
| /// Retrieve the [`SchemaProvider`] for a specific [`TableReference`], if it | ||
| /// esists. | ||
| pub fn schema_for_ref( |
There was a problem hiding this comment.
Not sure if there is a way in which we could keep this method as pub(crate), but I don't see many reasons to make this method not public.
notfilippo
changed the title
|
I've updated this PR to use a trait with the required method and not the SessionState |
There was a problem hiding this comment.
Thanks @notfilippo -- I think this structure looks good to me and it is consistent with other trait based APIs in the system
I have a few comments that might help about naming, but I think the documentation is what is really important to do. However, I also think we could do it as a follow on PR and this PR could be merged as is.
| }; | ||
|
|
||
| #[async_trait] | ||
| pub trait Context: Sync + Send + FunctionRegistry { |
There was a problem hiding this comment.
Could we please add some documentation here explaining
- What this trait is used for (provide the needed API to substrait encoding/deocing)
- That it is implemented for SessionState?
Also as Context is fairly generic, perhaps we could rename it to something like SubstraitContext or SubstraitPlanningContext
| } | ||
|
|
||
| #[async_trait] | ||
| impl Context for SessionState { |
There was a problem hiding this comment.
What would you think about also impl Context for SessionContext ? That would minimize the potential API changes on downstream users (also it would likely allow you to avoid many of the changes in this PR to the tests)?
Which issue does this PR close?
Closes #13342.
Rationale for this change
See original issue.
What changes are included in this PR?
See original issue.
Are these changes tested?
✅
Are there any user-facing changes?
Yes, the arguments for the substrait consumer / producer are changed. I could ideally provide some deprecated shim methods to provide backwards compatibility.