Skip to content

feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values #6196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 4 commits into from
May 4, 2023
Merged

feat: allow scalars to appear as the LHS arg in arithmetic operations on temporal values #6196

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24d8c27
feat: allow scalars to appear as the LHS arg in arithmetic operations…
kyle-mccarthy May 3, 2023
a0d4f88
fix: remove test from PR - another PR (#6201) adds equiv test
kyle-mccarthy May 4, 2023
8b7479c
Merge remote-tracking branch 'apache/main' into issue/6155
alamb May 4, 2023
3f785d3
fix clippy
alamb May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions datafusion/physical-expr/src/expressions/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@alamb alamb May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// 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(
Copy link
Contributor

@berkaysynnada berkaysynnada May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alamb alamb May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@berkaysynnada berkaysynnada May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

&array_lhs, sign, &array_rhs,
)?))
}
// This function evaluates temporal array operations, such as timestamp - timestamp, interval + interval,
// timestamp + interval, and interval + timestamp. It takes two arrays as input and an integer sign representing
// the operation (+1 for addition and -1 for subtraction).
(ColumnarValue::Array(array_lhs), ColumnarValue::Array(array_rhs)) => Ok(
ColumnarValue::Array(resolve_temporal_op(&array_lhs, sign, &array_rhs)?),
),
(_, _) => {
let msg = "If RHS of the operation is an array, then LHS also must be";
Err(DataFusionError::Internal(msg.to_string()))
}
}
}

Expand Down