You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kyle-mccarthy (via GitHub)" <gi...@apache.org> on 2023/05/03 03:58:43 UTC
[GitHub] [arrow-datafusion] kyle-mccarthy opened a new pull request, #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
kyle-mccarthy opened a new pull request, #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196
# Which issue does this PR close?
Closes #6155
# Rationale for this change
Currently, scalars cannot appear as the LHS arg in arithmetic operations on temporal values. This updates the relevant logic to accept scalars as the first argument.
# What changes are included in this PR?
Updates the temporal arithmetic operations to accept scalars as the LHS arg.
# Are these changes tested?
A test has been added for this new case.
# Are there any user-facing changes?
No
--
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 #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#discussion_r1184201432
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -120,16 +120,21 @@ impl PhysicalExpr for DateTimeIntervalExpr {
(ColumnarValue::Array(array_lhs), ColumnarValue::Scalar(array_rhs)) => {
resolve_temporal_op_scalar(&array_lhs, sign, &array_rhs)
}
+ // This function evaluates operations between a scalar value and an array of temporal
+ // values. One example is calculating the duration between a scalar timestamp and an
+ // array of timestamps (i.e. `now() - some_column`).
+ (ColumnarValue::Scalar(scalar_lhs), ColumnarValue::Array(array_rhs)) => {
+ let array_lhs = scalar_lhs.to_array_of_size(array_rhs.len());
+ Ok(ColumnarValue::Array(resolve_temporal_op(
Review Comment:
@berkaysynnada are you suggesting we should change this PR? Or can it be merged as is?
I am not quite sure what action your comment is suggesting
--
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] berkaysynnada commented on a diff in pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#discussion_r1184291958
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -120,16 +120,21 @@ impl PhysicalExpr for DateTimeIntervalExpr {
(ColumnarValue::Array(array_lhs), ColumnarValue::Scalar(array_rhs)) => {
resolve_temporal_op_scalar(&array_lhs, sign, &array_rhs)
}
+ // This function evaluates operations between a scalar value and an array of temporal
+ // values. One example is calculating the duration between a scalar timestamp and an
+ // array of timestamps (i.e. `now() - some_column`).
+ (ColumnarValue::Scalar(scalar_lhs), ColumnarValue::Array(array_rhs)) => {
+ let array_lhs = scalar_lhs.to_array_of_size(array_rhs.len());
+ Ok(ColumnarValue::Array(resolve_temporal_op(
Review Comment:
> @berkaysynnada are you suggesting we should change this PR? Or can it be merged as is?
>
> I am not quite sure what action your comment is suggesting
There is an operation creating an array from the scalar value. My suggestions intended to prevent this. I think we can merge this PR, I can update that part with a quick PR in the following days.
--
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] kyle-mccarthy commented on pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "kyle-mccarthy (via GitHub)" <gi...@apache.org>.
kyle-mccarthy commented on PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#issuecomment-1533900036
@alamb thanks for the review! I removed the test from this PR and noted in the commit that an equivalent test will be added in #6201
--
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] berkaysynnada commented on a diff in pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#discussion_r1183937192
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -120,16 +120,21 @@ impl PhysicalExpr for DateTimeIntervalExpr {
(ColumnarValue::Array(array_lhs), ColumnarValue::Scalar(array_rhs)) => {
resolve_temporal_op_scalar(&array_lhs, sign, &array_rhs)
}
+ // This function evaluates operations between a scalar value and an array of temporal
+ // values. One example is calculating the duration between a scalar timestamp and an
+ // array of timestamps (i.e. `now() - some_column`).
+ (ColumnarValue::Scalar(scalar_lhs), ColumnarValue::Array(array_rhs)) => {
+ let array_lhs = scalar_lhs.to_array_of_size(array_rhs.len());
+ Ok(ColumnarValue::Array(resolve_temporal_op(
Review Comment:
The purpose of the `resolve_temporal_op` is actually to work with real array types. Otherwise, would we need specialized handlers for scalar types? I think this case can be handled in two ways:
1) `resolve_temporal_op_scalar` is extended with a commute parameter. This pattern is applied in `impl_op_arithmetic`. The same functions should be able to handle Scalar op Array and Array op Scalar.
2) A similar resolve_temporal_op_scalar function can be added with replaced parameters of Array and Scalar.
Thank you for working on this issue. I can assist further if needed.
--
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 #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#discussion_r1183530222
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -772,4 +778,73 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_subtract_array_from_scalar() -> Result<()> {
Review Comment:
This is a neat test but I think we may be able to write it more concisely and get better end to end coverage by using sqllogictest. I will explain how to do that in the review comments
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -120,16 +120,21 @@ impl PhysicalExpr for DateTimeIntervalExpr {
(ColumnarValue::Array(array_lhs), ColumnarValue::Scalar(array_rhs)) => {
resolve_temporal_op_scalar(&array_lhs, sign, &array_rhs)
}
+ // This function evaluates operations between a scalar value and an array of temporal
Review Comment:
👍
--
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] berkaysynnada commented on a diff in pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#discussion_r1183937192
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -120,16 +120,21 @@ impl PhysicalExpr for DateTimeIntervalExpr {
(ColumnarValue::Array(array_lhs), ColumnarValue::Scalar(array_rhs)) => {
resolve_temporal_op_scalar(&array_lhs, sign, &array_rhs)
}
+ // This function evaluates operations between a scalar value and an array of temporal
+ // values. One example is calculating the duration between a scalar timestamp and an
+ // array of timestamps (i.e. `now() - some_column`).
+ (ColumnarValue::Scalar(scalar_lhs), ColumnarValue::Array(array_rhs)) => {
+ let array_lhs = scalar_lhs.to_array_of_size(array_rhs.len());
+ Ok(ColumnarValue::Array(resolve_temporal_op(
Review Comment:
The purpose of the `resolve_temporal_op` is actually to work with real array types. Otherwise, would we need specialized handlers for scalar types? I think this case can be handled in two ways:
1) `resolve_temporal_op_scalar` is extended with a commute parameter. This pattern is applied in `impl_op_arithmetic`. The same functions should be able to handle Scalar op Array and Array op Scalar.
2) A similar resolve_temporal_op_scalar function can be added with replaced parameters of Array and Scalar.
Thank you for working on this issue. I can assist further if needed.
--
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 pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196#issuecomment-1534850079
Thanks @kyle-mccarthy -- I took the liberty of merging up from main and fixing a clippy issue. I'll then rebase https://github.com/apache/arrow-datafusion/pull/6201 on this one 👍
--
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 merged pull request #6196: feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6196:
URL: https://github.com/apache/arrow-datafusion/pull/6196
--
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