You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "nrc (via GitHub)" <gi...@apache.org> on 2024/07/16 23:18:57 UTC
[PR] Implement data_part for intervals [arrow-rs]
nrc opened a new pull request, #6071:
URL: https://github.com/apache/arrow-rs/pull/6071
# 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.
-->
Addresses https://github.com/apache/datafusion/issues/6327
Mentioned in #4969.
# Rationale for this change
Will support using `extract` with intervals in DataFusion. It's also desirable functionality in Arrow for it's own sake, although I don't have a directly motivating use case.
# Are there any user-facing changes?
The `date_part` function will now work with intervals
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2251199363
I took the liberty of merging up from main to pick up https://github.com/apache/arrow-rs/pull/6116
--
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
Re: [PR] Implement date_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "nrc (via GitHub)" <gi...@apache.org>.
nrc commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2243780223
@alamb @Jefffrey would either of you be able to review this PR please? (Sorry if I'm missing process for requesting a review or you're not the best folk, it's my first Arrow PR).
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "nrc (via GitHub)" <gi...@apache.org>.
nrc commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2231982483
https://github.com/apache/datafusion/pull/11501 demonstrates how this change supports `extract` on intervals in DataFusion.
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2251002466
> @alamb @Jefffrey would either of you be able to review this PR please? (Sorry if I'm missing process for requesting a review or you're not the best folk, it's my first Arrow PR).
No problem @nrc -- I am sorry for the delay in reviewing. I think @tustvold has less time to review PRs than he used to, and the rest of us are just trying to work through reviews as much as possible. Sadly, review bandwidth is almost always our most limited resource
I am checking it out now
--
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
Re: [PR] Implement date_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2254106684
Thanks again @nrc
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#issuecomment-2251218333
FYI @Jefffrey and @tustvold -- I'll plan to merge this tomorrow unless there are any concerns or anyone would like more time to reivew
--
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
Re: [PR] Implement data_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#discussion_r1691896946
##########
arrow-arith/src/temporal.rs:
##########
@@ -387,6 +400,88 @@ impl ExtractDatePartExt for PrimitiveArray<TimestampNanosecondType> {
}
}
+impl ExtractDatePartExt for PrimitiveArray<IntervalYearMonthType> {
+ fn date_part(&self, part: DatePart) -> Result<Int32Array, ArrowError> {
+ match part {
+ DatePart::Year => Ok(self.unary_opt(|d| Some(d / 12))),
Review Comment:
Makes sense as IntervalYearMonth stores the number of whole months: https://docs.rs/arrow/latest/arrow/datatypes/enum.IntervalUnit.html
👍
##########
arrow-arith/src/temporal.rs:
##########
@@ -1500,4 +1595,203 @@ mod tests {
ensure_returns_error(&Time64MicrosecondArray::from(vec![0]));
ensure_returns_error(&Time64NanosecondArray::from(vec![0]));
}
+
+ // IntervalDayTimeType week, day, hour, miute, second, mili, u, nano; invalid month, year; ignores the other part
+ // IntervalMonthDayNanoType year -> nano; days don't affect months, time doesn't affect days, time doesn't affect months (and vice versa)
+ #[test]
+ fn test_interval_year_month_array() {
+ let input: IntervalYearMonthArray = vec![0, 5, 24].into();
+
+ let actual = date_part(&input, DatePart::Year).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(2, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Month).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(5, actual.value(1));
+ assert_eq!(0, actual.value(2));
+
+ assert!(date_part(&input, DatePart::Day).is_err());
+ assert!(date_part(&input, DatePart::Week).is_err());
+ }
+
+ #[test]
+ fn test_interval_day_time_array() {
+ let input: IntervalDayTimeArray = vec![
+ IntervalDayTime::ZERO,
+ IntervalDayTime::new(10, 42),
+ IntervalDayTime::new(10, 1042),
+ IntervalDayTime::new(10, MILLISECONDS_IN_DAY as i32 + 1),
+ ]
+ .into();
+
+ // Time doesn't affect days.
+ let actual = date_part(&input, DatePart::Day).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(10, actual.value(1));
+ assert_eq!(10, actual.value(2));
+ assert_eq!(10, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Week).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(1, actual.value(1));
+ assert_eq!(1, actual.value(2));
+ assert_eq!(1, actual.value(3));
+
+ // Days doesn't affect time.
+ let actual = date_part(&input, DatePart::Nanosecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000_000, actual.value(1));
+ assert_eq!(1_042_000_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Microsecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000, actual.value(1));
+ assert_eq!(1_042_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Millisecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42, actual.value(1));
+ assert_eq!(1042, actual.value(2));
+ assert_eq!(MILLISECONDS_IN_DAY as i32 + 1, actual.value(3));
Review Comment:
I find it somewhat strange that only for `MILLISECONDS_IN_DAY` we have a defined constant 🤔
##########
arrow-arith/src/temporal.rs:
##########
@@ -1500,4 +1595,203 @@ mod tests {
ensure_returns_error(&Time64MicrosecondArray::from(vec![0]));
ensure_returns_error(&Time64NanosecondArray::from(vec![0]));
}
+
+ // IntervalDayTimeType week, day, hour, miute, second, mili, u, nano; invalid month, year; ignores the other part
Review Comment:
these comments seem like they belong with the other tests `test_interval_day_time_array` and `test_interval_month_day_nano_array` 🤔
##########
arrow-arith/src/temporal.rs:
##########
@@ -1500,4 +1595,203 @@ mod tests {
ensure_returns_error(&Time64MicrosecondArray::from(vec![0]));
ensure_returns_error(&Time64NanosecondArray::from(vec![0]));
}
+
+ // IntervalDayTimeType week, day, hour, miute, second, mili, u, nano; invalid month, year; ignores the other part
+ // IntervalMonthDayNanoType year -> nano; days don't affect months, time doesn't affect days, time doesn't affect months (and vice versa)
+ #[test]
+ fn test_interval_year_month_array() {
+ let input: IntervalYearMonthArray = vec![0, 5, 24].into();
+
+ let actual = date_part(&input, DatePart::Year).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(2, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Month).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(5, actual.value(1));
+ assert_eq!(0, actual.value(2));
+
+ assert!(date_part(&input, DatePart::Day).is_err());
+ assert!(date_part(&input, DatePart::Week).is_err());
+ }
+
+ #[test]
+ fn test_interval_day_time_array() {
+ let input: IntervalDayTimeArray = vec![
+ IntervalDayTime::ZERO,
+ IntervalDayTime::new(10, 42),
+ IntervalDayTime::new(10, 1042),
+ IntervalDayTime::new(10, MILLISECONDS_IN_DAY as i32 + 1),
+ ]
+ .into();
+
+ // Time doesn't affect days.
+ let actual = date_part(&input, DatePart::Day).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(10, actual.value(1));
+ assert_eq!(10, actual.value(2));
+ assert_eq!(10, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Week).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(1, actual.value(1));
+ assert_eq!(1, actual.value(2));
+ assert_eq!(1, actual.value(3));
+
+ // Days doesn't affect time.
+ let actual = date_part(&input, DatePart::Nanosecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000_000, actual.value(1));
+ assert_eq!(1_042_000_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Microsecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000, actual.value(1));
+ assert_eq!(1_042_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Millisecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42, actual.value(1));
+ assert_eq!(1042, actual.value(2));
+ assert_eq!(MILLISECONDS_IN_DAY as i32 + 1, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Second).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(1, actual.value(2));
+ assert_eq!(24 * 60 * 60, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Minute).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(0, actual.value(2));
+ assert_eq!(24 * 60, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Hour).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(0, actual.value(2));
+ assert_eq!(24, actual.value(3));
+
+ // Month and year are not valid (since days in month varies).
+ assert!(date_part(&input, DatePart::Month).is_err());
+ assert!(date_part(&input, DatePart::Year).is_err());
+ }
+
+ #[test]
+ fn test_interval_month_day_nano_array() {
+ let input: IntervalMonthDayNanoArray = vec![
+ IntervalMonthDayNano::ZERO,
+ IntervalMonthDayNano::new(5, 10, 42),
+ IntervalMonthDayNano::new(16, 35, MILLISECONDS_IN_DAY * 1_000_000 + 1),
+ ]
+ .into();
+
+ // Year and month follow from month, but are not affected by days or nanos.
+ let actual = date_part(&input, DatePart::Year).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(1, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Month).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(5, actual.value(1));
+ assert_eq!(16, actual.value(2));
+
+ // Week and day follow from day, but are not affected by months or nanos.
+ let actual = date_part(&input, DatePart::Week).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(1, actual.value(1));
+ assert_eq!(5, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Day).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(10, actual.value(1));
+ assert_eq!(35, actual.value(2));
+
+ // Times follow from nanos, but are not affected by months or dats.
Review Comment:
```suggestion
// Times follow from nanos, but are not affected by months or 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
Re: [PR] Implement date_part for intervals [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6071:
URL: https://github.com/apache/arrow-rs/pull/6071#discussion_r1693936382
##########
arrow-arith/src/temporal.rs:
##########
@@ -1500,4 +1595,203 @@ mod tests {
ensure_returns_error(&Time64MicrosecondArray::from(vec![0]));
ensure_returns_error(&Time64NanosecondArray::from(vec![0]));
}
+
+ // IntervalDayTimeType week, day, hour, miute, second, mili, u, nano; invalid month, year; ignores the other part
+ // IntervalMonthDayNanoType year -> nano; days don't affect months, time doesn't affect days, time doesn't affect months (and vice versa)
+ #[test]
+ fn test_interval_year_month_array() {
+ let input: IntervalYearMonthArray = vec![0, 5, 24].into();
+
+ let actual = date_part(&input, DatePart::Year).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(2, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Month).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(5, actual.value(1));
+ assert_eq!(0, actual.value(2));
+
+ assert!(date_part(&input, DatePart::Day).is_err());
+ assert!(date_part(&input, DatePart::Week).is_err());
+ }
+
+ #[test]
+ fn test_interval_day_time_array() {
+ let input: IntervalDayTimeArray = vec![
+ IntervalDayTime::ZERO,
+ IntervalDayTime::new(10, 42),
+ IntervalDayTime::new(10, 1042),
+ IntervalDayTime::new(10, MILLISECONDS_IN_DAY as i32 + 1),
+ ]
+ .into();
+
+ // Time doesn't affect days.
+ let actual = date_part(&input, DatePart::Day).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(10, actual.value(1));
+ assert_eq!(10, actual.value(2));
+ assert_eq!(10, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Week).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(1, actual.value(1));
+ assert_eq!(1, actual.value(2));
+ assert_eq!(1, actual.value(3));
+
+ // Days doesn't affect time.
+ let actual = date_part(&input, DatePart::Nanosecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000_000, actual.value(1));
+ assert_eq!(1_042_000_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Microsecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42_000, actual.value(1));
+ assert_eq!(1_042_000, actual.value(2));
+ // Overflow returns zero.
+ assert_eq!(0, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Millisecond).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(42, actual.value(1));
+ assert_eq!(1042, actual.value(2));
+ assert_eq!(MILLISECONDS_IN_DAY as i32 + 1, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Second).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(1, actual.value(2));
+ assert_eq!(24 * 60 * 60, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Minute).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(0, actual.value(2));
+ assert_eq!(24 * 60, actual.value(3));
+
+ let actual = date_part(&input, DatePart::Hour).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(0, actual.value(2));
+ assert_eq!(24, actual.value(3));
+
+ // Month and year are not valid (since days in month varies).
+ assert!(date_part(&input, DatePart::Month).is_err());
+ assert!(date_part(&input, DatePart::Year).is_err());
+ }
+
+ #[test]
+ fn test_interval_month_day_nano_array() {
+ let input: IntervalMonthDayNanoArray = vec![
+ IntervalMonthDayNano::ZERO,
+ IntervalMonthDayNano::new(5, 10, 42),
+ IntervalMonthDayNano::new(16, 35, MILLISECONDS_IN_DAY * 1_000_000 + 1),
+ ]
+ .into();
+
+ // Year and month follow from month, but are not affected by days or nanos.
+ let actual = date_part(&input, DatePart::Year).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(0, actual.value(1));
+ assert_eq!(1, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Month).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(5, actual.value(1));
+ assert_eq!(16, actual.value(2));
+
+ // Week and day follow from day, but are not affected by months or nanos.
+ let actual = date_part(&input, DatePart::Week).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(1, actual.value(1));
+ assert_eq!(5, actual.value(2));
+
+ let actual = date_part(&input, DatePart::Day).unwrap();
+ let actual = actual.as_primitive::<Int32Type>();
+ assert_eq!(0, actual.value(0));
+ assert_eq!(10, actual.value(1));
+ assert_eq!(35, actual.value(2));
+
+ // Times follow from nanos, but are not affected by months or dats.
Review Comment:
Proposed to fix in https://github.com/apache/arrow-rs/pull/6140
--
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