You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2024/07/10 19:07:31 UTC
[PR] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
andygrove opened a new pull request, #11400:
URL: https://github.com/apache/datafusion/pull/11400
## 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.
-->
Part of https://github.com/apache/datafusion/issues/3520
## 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.
-->
In Comet, we are emulating Spark behavior and we need the ability to choose between failing on overflow or not. This seems like it is also important for Postgres compatibility, according to https://github.com/apache/datafusion/issues/3520
## 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.
-->
Add an option to `BinaryExpr` to control overflow behavior for some operators.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes, new tests added.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
No, the new flag defaults to false and is not wired into the planner yet.
<!--
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@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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#discussion_r1672834549
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -53,6 +53,8 @@ pub struct BinaryExpr {
left: Arc<dyn PhysicalExpr>,
op: Operator,
right: Arc<dyn PhysicalExpr>,
+ /// Specifies whether an error is returned on overflow or not
+ fail_on_overflow: bool,
Review Comment:
Comet can use this when constructing these physical expressions. For DataFusion, it can choose to set this flag on for ansi compliant cases/configs if any.
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#issuecomment-2221821005
> If there was a way to configure those (via argument to invoke, etc) or replace the singletons with a pre-configured instance (or better yet, referencing the one in the FunctionFactory) that would solve the issue.
How about overwrite existing function based on config?
```
set config.default
register "to_timestamp" with default_function
set config.spark
register "to_timestamp" with spark_function
set config.safe_mode
register "to_timestamp" with safe_to_timestamp_function
```
We can also overwrite packaged functions base on config to overwrite them, like what we have now
```rust
```
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#issuecomment-2221432353
Related: https://github.com/apache/datafusion/issues/10744
I have a personal branch where I[ maintain a 'safe' mode for to_timestamp ](https://github.com/Omega359/arrow-datafusion/blob/feature/safe_timestamp_and_date_parsing/datafusion/functions/src/datetime/to_timestamp.rs#L82)and to_date functions. It would be nice to get that sort of functionality into the core however I would only really want to do it if it could be driven via cofnig somehow. My attempts to make that work didn't pan out well and the best approach is likely one where invoke method is provided state (sessionState, SessionConfig, something) where we could query the config for flag(s) and adjust behaviour on the fly based on that.
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#issuecomment-2221538479
> Related: #10744
>
> I have a personal branch where I[ maintain a 'safe' mode for to_timestamp ](https://github.com/Omega359/arrow-datafusion/blob/feature/safe_timestamp_and_date_parsing/datafusion/functions/src/datetime/to_timestamp.rs#L82)and to_date functions. It would be nice to get that sort of functionality into the core however I would only really want to do it if it could be driven via config somehow. My attempts to make that work didn't pan out well and the best approach is likely one where invoke method is provided state (sessionState, SessionConfig, something) where we could query the config for flag(s) and adjust behaviour on the fly based on that.
Can you remind me why you need the physical expr to have access to configs at runtime rather than having the planner pass the relevant configs into the expressions when they are created?
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#discussion_r1672854921
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -62,7 +64,27 @@ impl BinaryExpr {
op: Operator,
right: Arc<dyn PhysicalExpr>,
) -> Self {
- Self { left, op, right }
+ Self {
+ left,
+ op,
+ right,
+ fail_on_overflow: false,
+ }
+ }
+
+ /// Create new binary expression with explicit fail_on_overflow value
Review Comment:
Another alternative API might be more of a builder style and easier to extend in the fuuure
```rust
// create expression
let expr = BinaryExpr::new(
Arc::new(Column::new("l", 0)),
Operator::Plus,
Arc::new(Column::new("r", 1)),
)
// configure to fail on overflow
.with_fail_on_overflow(true);
```
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#discussion_r1672832886
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2312,7 +2312,7 @@ mod tests {
// verify that the plan correctly casts u8 to i64
// the cast from u8 to i64 for literal will be simplified, and get lit(int64(5))
// the cast here is implicit so has CastOptions with safe=true
- let expected = "BinaryExpr { left: Column { name: \"c7\", index: 2 }, op: Lt, right: Literal { value: Int64(5) } }";
+ let expected = "BinaryExpr { left: Column { name: \"c7\", index: 2 }, op: Lt, right: Literal { value: Int64(5) }, fail_on_overflow: false }";
Review Comment:
This is coming from the derived `Debug` trait
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #11400:
URL: https://github.com/apache/datafusion/pull/11400
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#issuecomment-2221586240
> Can you remind me why you need the physical expr to have access to configs at runtime rather than having the planner pass the relevant configs into the expressions when they are created?
It's the singleton nature of UDF's currently - there isn't a way to configure them at all. You can replace a udf in the FunctionFactory with one with a different config which works well for sql however if you want to use the dataframe api with the standard function names such a `to_timestamp(args)` vs manually calling a modified function via something like `ctx.udf("to_timestamp").unwrap().call(args)` you can't as you cannot replace or modify what that function calls as it's a singleton
```
static $GNAME: std::sync::OnceLock<std::sync::Arc<datafusion_expr::ScalarUDF>> =
std::sync::OnceLock::new();
```
If there was a way to configure those (via argument to invoke, etc) or replace the singletons with a pre-configured instance (or better yet, referencing the one in the FunctionFactory) that would solve the issue.
--
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] feat: Add `fail_on_overflow` option to `BinaryExpr` [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #11400:
URL: https://github.com/apache/datafusion/pull/11400#discussion_r1672831102
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -2312,7 +2312,7 @@ mod tests {
// verify that the plan correctly casts u8 to i64
// the cast from u8 to i64 for literal will be simplified, and get lit(int64(5))
// the cast here is implicit so has CastOptions with safe=true
- let expected = "BinaryExpr { left: Column { name: \"c7\", index: 2 }, op: Lt, right: Literal { value: Int64(5) } }";
+ let expected = "BinaryExpr { left: Column { name: \"c7\", index: 2 }, op: Lt, right: Literal { value: Int64(5) }, fail_on_overflow: false }";
Review Comment:
I wonder if we want to display `fail_on_overflow`?
--
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