Repository navigation
feat!: pass input schema to ExprPlanner::plan_scalar - #26177
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26177 +/- ##
==========================================
- Coverage 82.78% 82.77% -0.01%
==========================================
Files 1148 1148
Lines 451075 451079 +4
Branches 451075 451079 +4
==========================================
- Hits 373400 373392 -8
- Misses 54927 54939 +12
Partials 22748 22748 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@alamb PTAL when you have time. |
alamb
left a comment
There was a problem hiding this comment.
Thank you @fengys1996 -- this PR makes sense to me; I think a small note in the upgrade guide, and perhaps a small example use / test that shows using the input schema would be valuable.
| /// | ||
| /// Returns the original scalar function if not possible | ||
| fn plan_scalar(&self, expr: RawScalarExpr) -> Result<PlannerResult<RawScalarExpr>> { | ||
| fn plan_scalar( |
There was a problem hiding this comment.
can you please add a note in the upgrade guide for this change as it is an API change?
There was a problem hiding this comment.
I took another look and checked the 55.0.0 and 55.2.0 tags, and ExprPlanner::plan_scalar isn't present in either. It was introduced in #24846 for the upcoming 56.0.0 release, so this signature change doesn't affect an API from a previous release. Given that, could we consider this PR non-breaking? 😄
|
FYI @MichaelScofield |
Which issue does this PR close?
Rationale for this change
Scalar function planners need access to the input schema to inspect argument types and field metadata, enabling expression rewrites.
What changes are included in this PR?
Add a
&DFSchemaparameter toExprPlanner::plan_scalarand pass the input schema during SQL planning.What is the testing strategy for this PR?
Updated
select_scalar_func_with_expr_plannerto verify that the planner can resolve a column argument’s type using the input schema and correctly rewrite the function call.Are there any user-facing changes?
Yes. Implementations overriding
ExprPlanner::plan_scalarmust accept an additional&DFSchemaparameter. This is a breaking API change.