You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/27 15:16:54 UTC
[GitHub] [arrow-datafusion] andygrove opened a new pull request, #2357: simplify_expressions no longer panics on unsupported evaluations
andygrove opened a new pull request, #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes https://github.com/apache/arrow-datafusion/issues/2356
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Make optimizer more robust.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
simplify_expressions no longer panics on unsupported evaluations
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on a diff in pull request #2357: Implement physical planner support for DATE +/- INTERVAL
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357#discussion_r862049831
##########
datafusion/core/src/optimizer/simplify_expressions.rs:
##########
@@ -400,9 +402,9 @@ impl<'a> ConstEvaluator<'a> {
}
/// Internal helper to evaluates an Expr
- pub(crate) fn evaluate_to_scalar(&self, expr: Expr) -> Result<ScalarValue> {
+ pub(crate) fn evaluate_to_scalar(&self, expr: &Expr) -> Result<ScalarValue> {
if let Expr::Literal(s) = expr {
- return Ok(s);
+ return Ok(s.clone());
Review Comment:
@alamb I reverted this change and actually completely changed direction on this PR ... please see the updated PR description.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove merged pull request #2357: Implement physical planner support for DATE +/- INTERVAL
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2357: WIP: simplify_expressions no longer panics on unsupported evaluations
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357#discussion_r860189248
##########
datafusion/core/src/optimizer/simplify_expressions.rs:
##########
@@ -400,9 +402,9 @@ impl<'a> ConstEvaluator<'a> {
}
/// Internal helper to evaluates an Expr
- pub(crate) fn evaluate_to_scalar(&self, expr: Expr) -> Result<ScalarValue> {
+ pub(crate) fn evaluate_to_scalar(&self, expr: &Expr) -> Result<ScalarValue> {
if let Expr::Literal(s) = expr {
- return Ok(s);
+ return Ok(s.clone());
Review Comment:
The `clone` is unfortunate -- I wonder if there is some way to pass it back. Like for example a signature such as he following (aka return the `Expr` as the error if the evaluation doesn't work)?
Maybe with some debugging messages on error (via `debug!`) would be sufficent?
```rust
pub(crate) fn evaluate_to_scalar(&self, expr: Expr) -> std::result::Result<ScalarValue, Expr> {
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on a diff in pull request #2357: simplify_expressions no longer panics on unsupported evaluations
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357#discussion_r859932302
##########
datafusion/core/tests/sql/mod.rs:
##########
@@ -535,19 +535,32 @@ async fn plan_and_collect(ctx: &SessionContext, sql: &str) -> Result<Vec<RecordB
/// Execute query and return results as a Vec of RecordBatches
async fn execute_to_batches(ctx: &SessionContext, sql: &str) -> Vec<RecordBatch> {
let msg = format!("Creating logical plan for '{}'", sql);
- let plan = ctx.create_logical_plan(sql).expect(&msg);
+ let plan = ctx
+ .create_logical_plan(sql)
+ .map_err(|e| format!("{:?} at {}", e, msg))
Review Comment:
This makes the tests show the actual error that happened so makes debugging much easier
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2357: Implement physical planner support for DATE +/- INTERVAL
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2357:
URL: https://github.com/apache/arrow-datafusion/pull/2357#discussion_r862053645
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -964,7 +965,27 @@ pub fn create_physical_expr(
input_schema,
execution_props,
)?;
- binary(lhs, *op, rhs, input_schema)
+ match (
Review Comment:
I like this approach to use a specialized operator
I think over the long term, the ideal outcome would be to extend the arrow temporal kernels https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/temporal.rs with the actual calculation logic.
However, incubating the kernels within datafusion seems like a great idea
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org