You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "berkaysynnada (via GitHub)" <gi...@apache.org> on 2023/05/24 07:29:19 UTC
[GitHub] [arrow-datafusion] berkaysynnada opened a new pull request, #6433: Refactor temporal arithmetic
berkaysynnada opened a new pull request, #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433
# 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 #6155.
# 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.
-->
https://github.com/apache/arrow-datafusion/pull/6196 PR has solved the issue of temporal scalars cannot appear as the LHS argument in an arithmetic operation. But there is an unoptimized solution such as creating an array from a scalar value as the length of the RHS array, and apply array vs array operations. This PR solves this issue in a similar way of LHS:Array and RHS:Scalar arithmetic.
# 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.
-->
In the datetime.rs, there are two functions that handle arithmetic operations between arrays and arrays, and between scalars and arrays. To determine the appropriate operation to perform, a flag is used to hold information about the left-hand side (LHS) and right-hand side (RHS) operands.
In the kernel_arrow.rs, type matching is performed to identify the specific operation to execute. Finally, the actual computation is carried out in the corresponding function within the binary.rs.
# 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.
# 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] alamb commented on pull request #6433: Refactor temporal arithmetic
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1568820228
Thanks everyone!
--
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] jackwener commented on a diff in pull request #6433: Refactor temporal arithmetic
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1204153777
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1874,524 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
+ scalar: &ScalarValue,
+ sign: i32,
+ array: &ArrayRef,
+) -> Result<ArrayRef> {
+ let ret = match (scalar, array.data_type()) {
Review Comment:
same with above.
We can use use ScalarValue::* use DataType::* use IntervalUnit::* to simplify code
--
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] mustafasrepo commented on pull request #6433: Refactor temporal arithmetic
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1568535864
> Thank you @berkaysynnada -- I think this is an improvement. I hope we can eventually move some of this logic upstream into arrow-rs but for now it seems good in DataFusion
>
> @mustafasrepo if you are good with this PR, perhaps you can merge it?
Sure, I have reviewed this Pr in our internal repo.
--
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] ozankabak commented on a diff in pull request #6433: Refactor temporal arithmetic
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1206000707
##########
datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs:
##########
@@ -333,34 +337,88 @@ pub(crate) fn add_dyn_temporal(left: &ArrayRef, right: &ArrayRef) -> Result<Arra
}
}
-pub(crate) fn add_dyn_temporal_scalar(
+pub(crate) fn add_dyn_temporal_right_scalar(
Review Comment:
A question that comes to my mind about this approach is whether the unary minus on the LHS (the array) would result in unnecessary computation or not. Given the complexity of the types involved, I am not sure if the compiler would be smart enough to optimize things all the way and emit simple subtraction code (vs. negating the entire array and then summing again).
--
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 closed pull request #6433: Refactor temporal arithmetic
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada closed pull request #6433: Refactor temporal arithmetic
URL: https://github.com/apache/arrow-datafusion/pull/6433
--
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 #6433: Refactor temporal arithmetic
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1561772519
I plan to review this PR tomorrow. Perhaps @tustvold is interested as I believe he is planning some work to improve the temporal kernel support in arrow-rs
--
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 pull request #6433: Refactor temporal arithmetic
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1567239681
> > Thank you @berkaysynnada -- in general this code is looking quite good. I think it is structured nicely and is improving
> > My one big concern is that the new code seems to be mostly be untested -- specifically there are a lot of new functions and code that looks similar to me such as `ts_ms_interval_mdn_op` and `ts_us_interval_mdn_op`
> > Do you know if this code covered by existing tests?
> > I realize it might be repetitive to write tests for this code, but I think it is is really important to avoid regressions in the future. Perhaps there is some way to reduce the code replication (and therefore also the testing burden) 🤔
>
> Thank you for the review ☺️ I have applied your suggestions. I have added some tests that compare the results of a(ScalarValue) vs b(another ScalarValue) and a vs c(Array having one element and the same value of b). I thought this would suffice, as the ScalarValue tests are wide-ranging and testing edge cases. I have also tried to reduce arithmetic functions as much as possible, but I would appreciate it if you let me know if anything else catches your eye.
--
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 #6433: Refactor temporal arithmetic
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1205975000
##########
datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs:
##########
@@ -333,34 +337,88 @@ pub(crate) fn add_dyn_temporal(left: &ArrayRef, right: &ArrayRef) -> Result<Arra
}
}
-pub(crate) fn add_dyn_temporal_scalar(
+pub(crate) fn add_dyn_temporal_right_scalar(
Review Comment:
Since adding `interval + duration` is the same as adding `duration + interval` I wonder if we need both of these kernels? Instead, perhaps we can use the same pattern as used elsewhere in binary.rs and do something like:
Given: `a + b`:
* if `b` is a scalar, call `add_dyn_scalar(a, b)`
* if `a` is a scalar, call `add_dyn_scalar(b, a)`
Given: `a - b`:
* if `b` is a scalar, call `sub_dyn_scalar(a, b)`
* if `a` is a scalar, call `add_dyn_scalar(-b, a)`
🤔
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1087,84 +1092,69 @@ impl BinaryExpr {
scalar: ScalarValue,
result_type: &DataType,
) -> Result<Option<Result<ArrayRef>>> {
+ use Operator::*;
Review Comment:
❤️
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1519,18 +1599,85 @@ pub fn ts_scalar_interval_op(
array.data_type()
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles timestamp +/- interval operations where the former is
Review Comment:
Minor suggestion is to move these functions into datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs so they are more discoverable
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1812,500 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
Review Comment:
cc @tustvold I am not sure if these exist (yet) in arrow-rs but I think we should be working to port them upstream
--
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 pull request #6433: Refactor temporal arithmetic
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1567146425
> Thank you @berkaysynnada -- in general this code is looking quite good. I think it is structured nicely and is improving
>
> My one big concern is that the new code seems to be mostly be untested -- specifically there are a lot of new functions and code that looks similar to me such as `ts_ms_interval_mdn_op` and `ts_us_interval_mdn_op`
>
> Do you know if this code covered by existing tests?
>
> I realize it might be repetitive to write tests for this code, but I think it is is really important to avoid regressions in the future. Perhaps there is some way to reduce the code replication (and therefore also the testing burden) 🤔
Thank you for the review ☺️ I have applied your suggestions. I have added some tests that compares the results of a(ScalarValue) vs b(another ScalarValue) and a vs c(Array having one element and the same value of b). I thought this would suffice, as the ScalarValue tests are wide-ranging and testing edge cases. I have also tried to reduce arithmetic functions as much as possible, but I would appreciate it if you let me know if anything else catches your eye.
--
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] tustvold commented on a diff in pull request #6433: Refactor temporal arithmetic
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1206720256
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1812,500 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
Review Comment:
I intend to spend some time designing what this might look like next week
--
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 #6433: Refactor temporal arithmetic
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1204561173
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1874,524 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
+ scalar: &ScalarValue,
+ sign: i32,
+ array: &ArrayRef,
+) -> Result<ArrayRef> {
+ let ret = match (scalar, array.data_type()) {
+ // YearMonth op YearMonth
+ (
+ ScalarValue::IntervalYearMonth(Some(lhs)),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ let array = as_interval_ym_array(&array)?;
+ let ret: PrimitiveArray<IntervalYearMonthType> =
+ unary(array, |rhs| op_ym(*lhs, rhs, sign));
+ Arc::new(ret) as _
Review Comment:
Thank you for the review. I cannot figure out getting rid of `as _ `, but your other advice of adding namespaces has significantly decreased the number of lines.
--
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] jackwener commented on a diff in pull request #6433: Refactor temporal arithmetic
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1204162324
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1689,7 +1874,524 @@ pub fn interval_scalar_interval_op(
scalar.get_datatype(),
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
+}
+
+/// This function handles interval +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an interval array.
+pub fn scalar_interval_op_interval(
+ scalar: &ScalarValue,
+ sign: i32,
+ array: &ArrayRef,
+) -> Result<ArrayRef> {
+ let ret = match (scalar, array.data_type()) {
+ // YearMonth op YearMonth
+ (
+ ScalarValue::IntervalYearMonth(Some(lhs)),
+ DataType::Interval(IntervalUnit::YearMonth),
+ ) => {
+ let array = as_interval_ym_array(&array)?;
+ let ret: PrimitiveArray<IntervalYearMonthType> =
+ unary(array, |rhs| op_ym(*lhs, rhs, sign));
+ Arc::new(ret) as _
Review Comment:
`Arc::new(ret) as _` occurs many times, is there any way we can avoid it?
--
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] jackwener commented on a diff in pull request #6433: Refactor temporal arithmetic
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1204151448
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1519,18 +1628,94 @@ pub fn ts_scalar_interval_op(
array.data_type()
)))?,
};
- Ok(ColumnarValue::Array(ret))
+ Ok(ret)
}
-macro_rules! sub_interval_macro {
+/// This function handles timestamp +/- interval operations where the former is
+/// a scalar and the latter is an array, resulting in an array.
+pub fn scalar_ts_op_interval(
+ scalar: &ScalarValue,
+ sign: i32,
+ array: &ArrayRef,
+) -> Result<ArrayRef> {
+ let ret = match (scalar, array.data_type()) {
Review Comment:
We can use `use ScalarValue::*` `use DataType::*` `use IntervalUnit::*` to simplify code
--
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 #6433: Refactor temporal arithmetic
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#discussion_r1205972150
##########
datafusion/common/src/scalar.rs:
##########
@@ -1247,14 +1247,14 @@ pub fn calculate_naives<const TIME_MODE: bool>(
}
#[inline]
-pub fn date32_add(days: i32, scalar: &ScalarValue, sign: i32) -> Result<i32> {
+pub fn date32_op(days: i32, scalar: &ScalarValue, sign: i32) -> Result<i32> {
Review Comment:
👍 that is a nice change
--
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] mustafasrepo merged pull request #6433: Refactor temporal arithmetic
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo merged PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433
--
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