Change public simplify API and add a public coerce API #3758
Conversation
github-actions
bot
added
logical-expr
alamb
force-pushed
the
alamb/new_simplify_coerce_api
branch
from
6567e5e to
56f115b
Compare
| @@ -15,17 +15,24 @@ | |||
| // specific language governing permissions and limitations | |||
| // under the License. | |||
|
|
|||
| //! Expression simplifier | |||
| //! Expression simplification API | |||
There was a problem hiding this comment.
This PR looks bigger than it really is -- it moves a bunch of code around and there are a significant number of docstrings
| @@ -37,28 +44,41 @@ pub trait SimplifyInfo { | |||
| fn execution_props(&self) -> &ExecutionProps; | |||
| } | |||
|
|
|||
| /// trait for types that can be simplified | |||
There was a problem hiding this comment.
Removed in favor of calling ExprSimplifier::simplify directly
| // rather than creating an DFSchemaRef coerces rather than doing | ||
| // it manually. | ||
| // TODO ticekt | ||
| pub fn coerce(&self, expr: Expr, schema: DFSchemaRef) -> Result<Expr> { |
There was a problem hiding this comment.
Here is the new API for coercion
There was a problem hiding this comment.
Good call -- will file ticket.
| /// let simplified = simplifier.simplify(expr).unwrap(); | ||
| /// assert_eq!(simplified, col("b").lt(lit(2))); | ||
| /// ``` | ||
| pub struct SimplifyContext<'a> { |
There was a problem hiding this comment.
This was moved into the public API and I added some docs and a builder interface with_schema
| }; | ||
| use datafusion_physical_expr::{create_physical_expr, execution_props::ExecutionProps}; | ||
|
|
||
| /// Provides simplification information based on schema and properties |
| @@ -950,30 +909,12 @@ macro_rules! assert_contains { | |||
| }; | |||
| } | |||
|
|
|||
| /// Apply simplification and constant propagation to ([Expr]). | |||
There was a problem hiding this comment.
Instead of adding a simplify method on to Expr via this trait, I propose to have an ExprSimplifier struct that has a simplify method.
cc @ygf11
I found it made the examples in expr_api.rs less awkward to write because the schema wasn't needed
There was a problem hiding this comment.
Looks great. It is more flexible, since user can define their SimplifyInfo other than inner SimplifyContext now.
alamb
changed the title
| /// See the [type coercion module](datafusion_expr::type_coercion) | ||
| /// documentation for more details on type coercion | ||
| /// | ||
| // Would be nice if this API could use the SimplifyInfo |
There was a problem hiding this comment.
If this PR is approved, I will file a ticket to remove the schema argument in this function call.
|
I plan on reviewing this one tomorrow @alamb |
Thank you @andygrove - I will have it ready and I'll also help try and review the outstanding DataFusion PRs as well |
|
This is blocking me upgrading DataFusion in my project (IOx). I know @andygrove plans to review today. Unless I hear otherwise I'll plan to merge PR tomorrow. |
|
I am happy to respond to comments / make changes as a follow on PR |
|
Benchmark runs are scheduled for baseline = fb39d5d and contender = 61c38b7. 61c38b7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3708
re #3709
Rationale for this change
See #3708
I am not sure if anyone other than IOx calls the expression simplification directly, but we do and I have had to change it non trivially now that type coercion is required prior to simplification (b/c it is required to run the constant evaluator)
What changes are included in this PR?
ExprSimplifierstructSimplfyContextExprSimplifierExprSimplifier::coercefunction and testsNote I plan to add better documentation / examples via #3740 / #3741
Follow on API improvements proposed in #3793
Are there any user-facing changes?
New API, changes to existing simplify API (I will highlight the changes inline)