feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values #6196
Conversation
… on temporal values
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
The code looks perfect! Thank you @kyle-mccarthy 👏
In terms of improving the test coverage, I think we should use a sqllogictest instead of using rust tests (though I realize you were simply following the model of existing tests)
The sqllogictest is here:
https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests
While working on a suggestion for you, I couldn't find existing sql level arithmetic tests, so I wrote some in: #6201
Thus I suggest you remove the tests from this PR, and we can use the test sin #6201 to verify your fix
| (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 |
| } | ||
|
|
||
| #[test] | ||
| fn test_subtract_array_from_scalar() -> Result<()> { |
There was a problem hiding this 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
| // 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( |
There was a problem hiding this 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:
resolve_temporal_op_scalaris extended with a commute parameter. This pattern is applied inimpl_op_arithmetic. The same functions should be able to handle Scalar op Array and Array op Scalar.- 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.
There was a problem hiding this 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 was a problem hiding this 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.
|
Thanks @kyle-mccarthy -- I took the liberty of merging up from main and fixing a clippy issue. I'll then rebase #6201 on this one 👍 |
3 participants
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