fix: add an "expr_planners" method to SessionState #15119
Conversation
niebayes
changed the title
niebayes
changed the title
|
CI appears to be failing so marking as draft |
| @@ -1632,7 +1632,7 @@ impl FunctionRegistry for SessionContext { | |||
| } | |||
|
|
|||
| fn expr_planners(&self) -> Vec<Arc<dyn ExprPlanner>> { | |||
| self.state.read().expr_planners() | |||
| self.state.read().expr_planners().to_vec() | |||
There was a problem hiding this comment.
Previously, it's the FunctionRegistry::expr_planners method gets called and a vector of expression planners is returned.
Now, the SessionState::expr_planners takes precedence and a slice of expression planners is returned.
There was a problem hiding this comment.
@alamb Fixed CI. PTAL
There was a problem hiding this comment.
I don't understand how the code in this PR is any different than what is in on main
There was a problem hiding this comment.
Well, both the FunctionRegistry and SessionState provide an expr_planners method, but they return different data type. FunctionRegistry::expr_planners returns Vec<dyn ExprPlanner, and SessionState::expr_planners returns &[dyn ExprPlanner].
Now, the SessionState::expr_planners takes precedence and a slice of expression planners is returned. Therefore, we have add a to_vec.
There was a problem hiding this comment.
I think it's ContextProvider that uses a slice vs Vec. Bit annoying the types are not aligned. SessionState uses expr_planners: Vec<Arc<dyn ExprPlanner>>,
There was a problem hiding this comment.
@Omega359 I agree, it's better to align the types.
I tried to align the types but ran into some difficulties. If both functions return a slice, SessionContext::expr_planners would encounter the error Cannot return a reference of temporary values.
If both return a Vec, then SessionState's implementation of ContextProvider::get_expr_planners would also face the same issue. The only way to resolve this would be to change the return type of ContextProvider::get_expr_planners to Vec, but that would be a breaking change.
|
@alamb I wonder if we can remove the |
Actually, I thought the idea was that SessionState implemented FunctionRegistry or something like that |
|
@alamb Sure, I maybe write an example to demonstrate how why the newly added |
|
@alamb Sure, I maybe write an example to demonstrate how why the newly added expr_planners method is helpful What I am thinking is "how will we ensure that we don't accidentally break this feature during future refactors" As it stands now, we could potentially break the feature without any tests failing Ideally we would have some test that fails so we would know to fix it |
alamb
changed the title
|
I plan to make a 46.0.1 release tomorrow Is this something we should include? Or is there a workaround? |
|
@alamb Sorry for late response, Some emergent personal matters have caused a delay. I will complete this in the next few hours. |
niebayes
force-pushed
the
chore/add_expr_planners_api
branch
from
3bbff38 to
40a717b
Compare
|
@alamb Hi, I've written a test to demonstrate why it's more convenient and somewhat necessary to provide an I'm not sure if this test clearly conveyed my thoughts. Appreciate any feedback. |
| }, | ||
| physical_plan::ExecutionPlan, | ||
| sql::{planner::ContextProvider, ResolvedTableReference, TableReference}, | ||
| }; |
There was a problem hiding this comment.
You may as well pull in the other 2 crate lines above if you are going to use this import style.
There was a problem hiding this comment.
@Omega359 I have flattened all imports.
Which issue does this PR close?
Rationale for this change
This method makes it convenient for users to retrieve expression planners from SessionState. The details are explained in the corresponding issue.
What changes are included in this PR?
Added an
expr_plannersto SessionState.Are these changes tested?
Covered by existing test cases.
Are there any user-facing changes?
Yes, users now can use the
expr_plannersmethod to get registered expression planners, including default expression planners, from SessionState.