You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/05/01 17:23:53 UTC
[PR] Improve coerce API so it does not need DFSchema [datafusion]
alamb opened a new pull request, #10331:
URL: https://github.com/apache/datafusion/pull/10331
## Which issue does this PR close?
Closes https://github.com/apache/datafusion/issues/3793
## Rationale for this change
1. Permit making an API for https://github.com/apache/datafusion/issues/10181 that takes a `&DFSchema` rather than an owned
2. Avoid a copy and thus make the API marginally faster in some cases.
## What changes are included in this PR?
Change `coerce` API from
```
pub fn coerce(&self, expr: Expr, schema: DFSchemaRef) -> Result<Expr> {
```
to
```rust
pub fn coerce(&self, expr: Expr, schema: &DFSchema) -> Result<Expr> {
```
## Are these changes tested?
By existing CI
## Are there any user-facing changes?
The API to call coerce is slightly changed. Note I am in the process of adding a proper fully baked API in https://github.com/apache/datafusion/issues/10181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve coerce API so it does not need DFSchema [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10331:
URL: https://github.com/apache/datafusion/pull/10331#discussion_r1586538159
##########
datafusion-examples/examples/expr_api.rs:
##########
@@ -258,7 +258,7 @@ pub fn physical_expr(schema: &Schema, expr: Expr) -> Result<Arc<dyn PhysicalExpr
ExprSimplifier::new(SimplifyContext::new(&props).with_schema(df_schema.clone()));
// apply type coercion here to ensure types match
- let expr = simplifier.coerce(expr, df_schema.clone())?;
+ let expr = simplifier.coerce(expr, &df_schema)?;
Review Comment:
This shows the effect of this API change on users (add a &)
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -208,14 +206,8 @@ impl<S: SimplifyInfo> ExprSimplifier<S> {
///
/// 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
- // rather than creating an DFSchemaRef coerces rather than doing
- // it manually.
- // https://github.com/apache/datafusion/issues/3793
- pub fn coerce(&self, expr: Expr, schema: DFSchemaRef) -> Result<Expr> {
+ pub fn coerce(&self, expr: Expr, schema: &DFSchema) -> Result<Expr> {
Review Comment:
This is the actual change (I don't think making it take `SimplifyInfo` is practical given how much coerce does)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve coerce API so it does not need DFSchema [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10331:
URL: https://github.com/apache/datafusion/pull/10331#issuecomment-2090274310
Thank you for the review @jayzhan211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve coerce API so it does not need DFSchema [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10331:
URL: https://github.com/apache/datafusion/pull/10331
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org