You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/08 17:46:20 UTC
[GitHub] [arrow-rs] avantgardnerio opened a new pull request, #2031: Add support for adding intervals to dates
avantgardnerio opened a new pull request, #2031:
URL: https://github.com/apache/arrow-rs/pull/2031
Closes #527.
# Rationale for this change
Support for adding scalar intervals to scalar dates was added in datafusion, but in order to support adding columns to columns, we need to move that logic down into an arrow kernel.
# What changes are included in this PR?
- Support for adding interval types to date types
- A change to `add_dyn` to allow it to accept lambda functions with two differing parameter types
# Are there any user-facing changes?
Adding intervals to dates should work 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
[GitHub] [arrow-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917132616
##########
arrow/Cargo.toml:
##########
@@ -51,8 +51,9 @@ csv_crate = { version = "1.1", default-features = false, optional = true, packag
regex = { version = "1.5.6", default-features = false, features = ["std", "unicode"] }
lazy_static = { version = "1.4", default-features = false }
packed_simd = { version = "0.3", default-features = false, optional = true, package = "packed_simd_2" }
-chrono = { version = "0.4", default-features = false, features = ["clock"] }
+chrono = { version = "0.4", default-features = false, features = ["std", "clock"] }
chrono-tz = {version = "0.6", default-features = false, optional = true}
+chronoutil = "0.2.3"
Review Comment:
It has been inlined
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917152007
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
Review Comment:
Yes, I struggled a lot with this. It's very confusing the distinction between these. It technically returns a `<Date64Type as IntervalYearMonthType>::Native` which is just an `i32`. I question where to even put these methods, or if they belong on this impl, or what to call them in the first place. I think the awkwardness comes from arrow not really having a row, so heretofor there was nothing that operated on an individual value, but also thus some bad tests that did not properly parse/populate these, so I think some standard way to convert would be very helpful, hence the addition.
--
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-rs] avantgardnerio commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1182084899
> @avantgardnerio It's a flaky test #1931 that should have now been fixed [apache/arrow#13573](https://github.com/apache/arrow/pull/13573). I'll rerun the CI job and it should clear
Looks great, thanks!
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r920331501
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
Review Comment:
Yea, makes sense to me.
--
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-rs] avantgardnerio commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1213233374
> if you are tracking
I was not, but am now, ty!
--
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-rs] andygrove merged pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
andygrove merged PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031
--
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-rs] andygrove commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917031658
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -914,6 +963,60 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 02, 01)));
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 01, 02)));
+ }
+
+ #[test]
+ fn test_date32_month_day_nano_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNanoType::from(1, 1, 1)]);
Review Comment:
nit: it would be easier to determine if the test is correct if we used different values for each component
```suggestion
let b = IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNanoType::from(1, 2, 3)]);
```
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917156004
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
+ ///
+ /// # Arguments
+ ///
+ /// * `years` - The number of years (+/-) represented in this interval
+ /// * `months` - The number of months (+/-) represented in this interval
+ pub fn new(
+ years: i32,
+ months: i32,
+ ) -> <IntervalYearMonthType as ArrowPrimitiveType>::Native {
+ years * 12 + months
+ }
+
+ pub fn to_months(i: <IntervalYearMonthType as ArrowPrimitiveType>::Native) -> i32 {
+ i
+ }
+}
+
+impl IntervalDayTimeType {
+ /// Creates a IntervalDayTimeType
+ ///
+ /// # Arguments
+ ///
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `millis` - The number of milliseconds (+/-) represented in this interval
+ pub fn new(
+ days: i32,
+ millis: i32,
+ ) -> <IntervalDayTimeType as ArrowPrimitiveType>::Native {
+ let m = millis as u64 & u32::MAX as u64;
+ let d = (days as u64 & u32::MAX as u64) << 32;
+ (m | d) as <IntervalDayTimeType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
Review Comment:
If there's a `pub` keyword, doc it - got it. Done.
--
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-rs] alamb commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917095969
##########
arrow/Cargo.toml:
##########
@@ -51,8 +51,9 @@ csv_crate = { version = "1.1", default-features = false, optional = true, packag
regex = { version = "1.5.6", default-features = false, features = ["std", "unicode"] }
lazy_static = { version = "1.4", default-features = false }
packed_simd = { version = "0.3", default-features = false, optional = true, package = "packed_simd_2" }
-chrono = { version = "0.4", default-features = false, features = ["clock"] }
+chrono = { version = "0.4", default-features = false, features = ["std", "clock"] }
chrono-tz = {version = "0.6", default-features = false, optional = true}
+chronoutil = "0.2.3"
Review Comment:
As I mentioned on https://github.com/apache/arrow-datafusion/pull/2797#discussion_r917090447 the only thing I am worried about is this new dependency (I am trying to avoid too many dependencies).
What do other reviewers think about inlining the (small) functions used in this crate into arrow rather than adding a new dependency?
quoting https://go-proverbs.github.io/
> [A little copying is better than a little dependency.](https://www.youtube.com/watch?v=PAAkCSZUG1c&t=9m28s)
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -774,6 +778,110 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
DataType::Dictionary(_, _) => {
typed_dict_math_op!(left, right, |a, b| a + b, math_op_dict)
}
+ DataType::Date32 => {
+ let l = left
+ .as_any()
+ .downcast_ref::<PrimitiveArray<Date32Type>>()
+ .ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to Date32Type",
+ ))
+ })?;
Review Comment:
For what it is worth, most of the rest of the codebase simply `panic`s if the types don't match (as it is a fairly large runtime invariant).
So a common pattern is like:
```suggestion
let l = left
.as_any()
.downcast_ref::<PrimitiveArray<Date32Type>>()
.unwrap();
```
There is also https://docs.rs/arrow/17.0.0/arrow/array/fn.as_primitive_array.html# to make things even simpler:
```
let l = as_primitive_array<Date32Type>(&left);
```
##########
arrow/Cargo.toml:
##########
@@ -51,8 +51,9 @@ csv_crate = { version = "1.1", default-features = false, optional = true, packag
regex = { version = "1.5.6", default-features = false, features = ["std", "unicode"] }
lazy_static = { version = "1.4", default-features = false }
packed_simd = { version = "0.3", default-features = false, optional = true, package = "packed_simd_2" }
-chrono = { version = "0.4", default-features = false, features = ["clock"] }
+chrono = { version = "0.4", default-features = false, features = ["std", "clock"] }
Review Comment:
I know we have had some users of arrow try to use arrow in webasm (where `std` is not available), so I am a bit worried about this change. However, I think we have good CI coverage of this so if the CI checks pass this change should be fine.
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -55,14 +58,15 @@ use std::sync::Arc;
/// # Errors
///
/// This function errors if the arrays have different lengths
-pub fn math_op<T, F>(
- left: &PrimitiveArray<T>,
- right: &PrimitiveArray<T>,
+pub fn math_op<LT, RT, F>(
Review Comment:
👍
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,147 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ pub fn from(
Review Comment:
The same comment applies to the other `from` methods
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,147 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ pub fn from(
Review Comment:
I think we should add adding some docstrings to thee functions
Also, I think a more idiomatic rust API would be to call this function `new()` -- as `from` is part of the standard https://doc.rust-lang.org/std/convert/trait.From.html trait
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917111670
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -774,6 +778,110 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
DataType::Dictionary(_, _) => {
typed_dict_math_op!(left, right, |a, b| a + b, math_op_dict)
}
+ DataType::Date32 => {
+ let l = left
+ .as_any()
+ .downcast_ref::<PrimitiveArray<Date32Type>>()
+ .ok_or_else(|| {
+ ArrowError::CastError(format!(
+ "Left array cannot be cast to Date32Type",
+ ))
+ })?;
Review Comment:
Thank you, the verbosity was bugging the hell out of me.
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917156221
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -774,6 +778,54 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
DataType::Dictionary(_, _) => {
typed_dict_math_op!(left, right, |a, b| a + b, math_op_dict)
}
+ DataType::Date32 => {
+ let l = as_primitive_array::<Date32Type>(left);
+ match right.data_type() {
+ DataType::Interval(IntervalUnit::YearMonth) => {
+ let r = as_primitive_array::<IntervalYearMonthType>(right);
+ let res = math_op(l, r, Date32Type::add_year_months)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::DayTime) => {
+ let r = as_primitive_array::<IntervalDayTimeType>(right);
+ let res = math_op(l, r, Date32Type::add_day_time)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::MonthDayNano) => {
+ let r = as_primitive_array::<IntervalMonthDayNanoType>(right);
+ let res = math_op(l, r, Date32Type::add_month_day_nano)?;
+ Ok(Arc::new(res))
+ }
+ t => Err(ArrowError::CastError(format!(
+ "Cannot perform arithmetic operation on arrays of type {}",
+ t
+ ))),
Review Comment:
Fixed.
--
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-rs] alamb commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917257306
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
Sorry for the contradictory feedback -- I defer to @viirya in terms of using dependency or inlining. Given how little actual code it is, I kind of like the avoidance of a new dependency but I agree this is an opinion rather than anything driven by other considerations
--
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-rs] tustvold commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1182017722
@avantgardnerio It's a flaky test #1931 that should have now been fixed https://github.com/apache/arrow/pull/13573. I'll rerun the CI job and it should clear
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917148330
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
Review Comment:
Hm, looks like `new` returns the native value, not `IntervalYearMonthType` type?
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917041468
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -914,6 +963,60 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 02, 01)));
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 01, 02)));
+ }
+
+ #[test]
+ fn test_date32_month_day_nano_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNanoType::from(1, 1, 1)]);
Review Comment:
Done!
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -914,6 +963,60 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 02, 01)));
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 01, 02)));
Review Comment:
Fixed!
--
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-rs] alamb commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1185660670
🎉
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917147575
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
Why copying the code, instead of using it as dependency?
--
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-rs] andygrove commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
andygrove commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917030152
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -914,6 +963,60 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 02, 01)));
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 01, 02)));
Review Comment:
Is this the correct result? I wasn't expecting the year to be incremented here
--
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-rs] avantgardnerio commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1179317943
> Looks like there is a minor clippy error: https://github.com/apache/arrow-rs/runs/7256871633?check_suite_focus=true
I had to re-install ubuntu to get clippy to work, but the next iteration should pass.
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917132401
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,147 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ pub fn from(
Review Comment:
Sorry, I misunderstood, corrected 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
[GitHub] [arrow-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917107482
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,147 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ pub fn from(
Review Comment:
Unfortunately, I think the `to`/`from` traits would apply to the `::Native` things: `i32`, `i64`, etc :frowning_face: so I used these non-standard ones.
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917148781
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
+ ///
+ /// # Arguments
+ ///
+ /// * `years` - The number of years (+/-) represented in this interval
+ /// * `months` - The number of months (+/-) represented in this interval
+ pub fn new(
+ years: i32,
+ months: i32,
+ ) -> <IntervalYearMonthType as ArrowPrimitiveType>::Native {
+ years * 12 + months
+ }
+
+ pub fn to_months(i: <IntervalYearMonthType as ArrowPrimitiveType>::Native) -> i32 {
+ i
+ }
+}
+
+impl IntervalDayTimeType {
+ /// Creates a IntervalDayTimeType
+ ///
+ /// # Arguments
+ ///
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `millis` - The number of milliseconds (+/-) represented in this interval
+ pub fn new(
+ days: i32,
+ millis: i32,
+ ) -> <IntervalDayTimeType as ArrowPrimitiveType>::Native {
+ let m = millis as u64 & u32::MAX as u64;
+ let d = (days as u64 & u32::MAX as u64) << 32;
+ (m | d) as <IntervalDayTimeType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
Review Comment:
As public method, could you add a doc?
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
+ ///
+ /// # Arguments
+ ///
+ /// * `years` - The number of years (+/-) represented in this interval
+ /// * `months` - The number of months (+/-) represented in this interval
+ pub fn new(
+ years: i32,
+ months: i32,
+ ) -> <IntervalYearMonthType as ArrowPrimitiveType>::Native {
+ years * 12 + months
+ }
+
+ pub fn to_months(i: <IntervalYearMonthType as ArrowPrimitiveType>::Native) -> i32 {
+ i
+ }
+}
+
+impl IntervalDayTimeType {
+ /// Creates a IntervalDayTimeType
+ ///
+ /// # Arguments
+ ///
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `millis` - The number of milliseconds (+/-) represented in this interval
+ pub fn new(
+ days: i32,
+ millis: i32,
+ ) -> <IntervalDayTimeType as ArrowPrimitiveType>::Native {
+ let m = millis as u64 & u32::MAX as u64;
+ let d = (days as u64 & u32::MAX as u64) << 32;
+ (m | d) as <IntervalDayTimeType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
+ i: <IntervalDayTimeType as ArrowPrimitiveType>::Native,
+ ) -> (i32, i32) {
+ let days = (i >> 32) as i32;
+ let ms = i as i32;
+ (days, ms)
+ }
+}
+
+impl IntervalMonthDayNanoType {
+ /// Creates a IntervalMonthDayNanoType
+ ///
+ /// # Arguments
+ ///
+ /// * `months` - The number of months (+/-) represented in this interval
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `nanos` - The number of nanoseconds (+/-) represented in this interval
+ pub fn new(
+ months: i32,
+ days: i32,
+ nanos: i64,
+ ) -> <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native {
+ let m = months as u128 & u32::MAX as u128;
+ let d = (days as u128 & u32::MAX as u128) << 32;
+ let n = (nanos as u128) << 64;
+ (m | d | n) as <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
Review Comment:
ditto
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917150803
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
There was a conversation in the equivalent `arrow-rs` commit: https://github.com/apache/arrow-datafusion/pull/2797#discussion_r917090447
I would vote to make it a dependency, but am fine either way.
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917285082
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
I agree with @alamb. The code copied is little so not a strong concern from me. I think we can copy the code now and maybe consider to remove it by adding chrono as dependency if it shows stable maintenance in the future.
--
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-rs] alamb commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1179307683
Looks like there is a minor clippy error: https://github.com/apache/arrow-rs/runs/7256871633?check_suite_focus=true
--
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-rs] avantgardnerio commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1183169542
> renamed this method make_value
I think that is both valid, and non-trivial. I've renamed and pushed :slightly_smiling_face:
--
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-rs] avantgardnerio commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1182016546
Could anyone help me with reproducing this locally, or understanding what went wrong?
```
==========================================================
Testing file duration
==========================================================
Traceback (most recent call last):
File "/arrow/dev/archery/archery/integration/runner.py", line 246, in _run_ipc_test_case
run_binaries(producer, consumer, test_case)
File "/arrow/dev/archery/archery/integration/runner.py", line 286, in _produce_consume
consumer.stream_to_file(producer_stream_path, consumer_file_path)
File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 63, in stream_to_file
self.run_shell_command(cmd)
File "/arrow/dev/archery/archery/integration/tester.py", line 49, in run_shell_command
subprocess.check_call(cmd, shell=True)
File "/opt/conda/envs/arrow/lib/python3.10/subprocess.py", line 369, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/arrow/csharp/artifacts/Apache.Arrow.IntegrationTest/Debug/net6.0/Apache.Arrow.IntegrationTest --mode stream-to-file -a /tmp/tmpzg7b26id/29338ab4_generated_datetime.consumer_stream_as_file < /tmp/tmpzg7b26id/29338ab4_generated_datetime.producer_file_as_stream' returned non-zero exit status 1.
################# FAILURES #################
Traceback (most recent call last):
FAILED TEST: datetime Java producing, C# consuming
File "/arrow/dev/archery/archery/integration/runner.py", line 246, in _run_ipc_test_case
run_binaries(producer, consumer, test_case)
File "/arrow/dev/archery/archery/integration/runner.py", line 286, in _produce_consume
consumer.stream_to_file(producer_stream_path, consumer_file_path)
File "/arrow/dev/archery/archery/integration/tester_csharp.py", line 63, in stream_to_file
self.run_shell_command(cmd)
File "/arrow/dev/archery/archery/integration/tester.py", line 49, in run_shell_command
subprocess.check_call(cmd, shell=True)
File "/opt/conda/envs/arrow/lib/python3.10/subprocess.py", line 369, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/arrow/csharp/artifacts/Apache.Arrow.IntegrationTest/Debug/net6.0/Apache.Arrow.IntegrationTest --mode stream-to-file -a /tmp/tmpzg7b26id/4b457ee3_generated_datetime.consumer_stream_as_file < /tmp/tmpzg7b26id/4b457ee3_generated_datetime.producer_file_as_stream' returned non-zero exit status 1.
FAILED TEST: datetime Rust producing, C# consuming
```
Thanks!
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917031830
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -914,6 +963,60 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 02, 01)));
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 01, 01))]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::from(1, 1)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(c.value(0), Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 01, 02)));
Review Comment:
Just caught that myself... fixing!
--
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-rs] codecov-commenter commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1179371841
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2031](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2068071) into [master](https://codecov.io/gh/apache/arrow-rs/commit/ca1bfb8f28fcd82757ce08c8038916bb6e7986e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ca1bfb8) will **increase** coverage by `0.07%`.
> The diff coverage is `98.75%`.
```diff
@@ Coverage Diff @@
## master #2031 +/- ##
==========================================
+ Coverage 83.54% 83.61% +0.07%
==========================================
Files 222 223 +1
Lines 58178 58467 +289
==========================================
+ Hits 48604 48887 +283
- Misses 9574 9580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `99.24% <ø> (ø)` | |
| [arrow/src/compute/kernels/arithmetic.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9hcml0aG1ldGljLnJz) | `93.97% <97.53%> (+0.35%)` | :arrow_up: |
| [arrow/src/datatypes/types.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy90eXBlcy5ycw==) | `97.46% <98.57%> (+8.57%)` | :arrow_up: |
| [arrow/src/datatypes/delta.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kZWx0YS5ycw==) | `100.00% <100.00%> (ø)` | |
| [parquet/src/arrow/record\_reader/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci9tb2QucnM=) | `89.17% <0.00%> (-0.19%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (+0.22%)` | :arrow_up: |
| [parquet/src/arrow/arrow\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/2031/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfcmVhZGVyLnJz) | `92.67% <0.00%> (+0.27%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ca1bfb8...2068071](https://codecov.io/gh/apache/arrow-rs/pull/2031?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917149020
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
+ ///
+ /// # Arguments
+ ///
+ /// * `years` - The number of years (+/-) represented in this interval
+ /// * `months` - The number of months (+/-) represented in this interval
+ pub fn new(
+ years: i32,
+ months: i32,
+ ) -> <IntervalYearMonthType as ArrowPrimitiveType>::Native {
+ years * 12 + months
+ }
+
+ pub fn to_months(i: <IntervalYearMonthType as ArrowPrimitiveType>::Native) -> i32 {
+ i
+ }
+}
+
+impl IntervalDayTimeType {
+ /// Creates a IntervalDayTimeType
+ ///
+ /// # Arguments
+ ///
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `millis` - The number of milliseconds (+/-) represented in this interval
+ pub fn new(
+ days: i32,
+ millis: i32,
+ ) -> <IntervalDayTimeType as ArrowPrimitiveType>::Native {
+ let m = millis as u64 & u32::MAX as u64;
+ let d = (days as u64 & u32::MAX as u64) << 32;
+ (m | d) as <IntervalDayTimeType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
+ i: <IntervalDayTimeType as ArrowPrimitiveType>::Native,
+ ) -> (i32, i32) {
+ let days = (i >> 32) as i32;
+ let ms = i as i32;
+ (days, ms)
+ }
+}
+
+impl IntervalMonthDayNanoType {
+ /// Creates a IntervalMonthDayNanoType
+ ///
+ /// # Arguments
+ ///
+ /// * `months` - The number of months (+/-) represented in this interval
+ /// * `days` - The number of days (+/-) represented in this interval
+ /// * `nanos` - The number of nanoseconds (+/-) represented in this interval
+ pub fn new(
+ months: i32,
+ days: i32,
+ nanos: i64,
+ ) -> <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native {
+ let m = months as u128 & u32::MAX as u128;
+ let d = (days as u128 & u32::MAX as u128) << 32;
+ let n = (nanos as u128) << 64;
+ (m | d | n) as <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native
+ }
+
+ pub fn to_parts(
+ i: <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native,
+ ) -> (i32, i32, i64) {
+ let nanos = (i >> 64) as i64;
+ let days = (i >> 32) as i32;
+ let months = i as i32;
+ (months, days, nanos)
+ }
+}
+
+impl Date32Type {
Review Comment:
It's better to add doc to public methods.
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917145536
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -774,6 +778,54 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
DataType::Dictionary(_, _) => {
typed_dict_math_op!(left, right, |a, b| a + b, math_op_dict)
}
+ DataType::Date32 => {
+ let l = as_primitive_array::<Date32Type>(left);
+ match right.data_type() {
+ DataType::Interval(IntervalUnit::YearMonth) => {
+ let r = as_primitive_array::<IntervalYearMonthType>(right);
+ let res = math_op(l, r, Date32Type::add_year_months)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::DayTime) => {
+ let r = as_primitive_array::<IntervalDayTimeType>(right);
+ let res = math_op(l, r, Date32Type::add_day_time)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::MonthDayNano) => {
+ let r = as_primitive_array::<IntervalMonthDayNanoType>(right);
+ let res = math_op(l, r, Date32Type::add_month_day_nano)?;
+ Ok(Arc::new(res))
+ }
+ t => Err(ArrowError::CastError(format!(
+ "Cannot perform arithmetic operation on arrays of type {}",
+ t
+ ))),
Review Comment:
`t` will be right array type. So this error will be read a bit confusing, I think.
Maybe more specific? Like:
```suggestion
t => Err(ArrowError::CastError(format!(
"Cannot perform arithmetic operation between array of type {} and array of type {}",
left.data_type(), right.data_type()
))),
```
--
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-rs] alamb commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917257306
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
Sorry for the contradictory feedback -- I defer to @viirya in terms of using dependency or inlining. Given how little actual code it is, I kind of like the avoidance of a new dependency but I agree this is an opinion rather than anything driven by other considerations.
The reason I personally prefer to avoid dependencies are they are really hard to remove (e.g. https://github.com/apache/arrow-rs/issues/1882) and there are many of these rust crates which seem to have an initial spurt of brilliance and then basically become unmaintained as the authors move on to something else. Though to be honest chrono itself kind of falls into that category too 🤔
--
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-rs] alamb commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r919926663
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1068,6 +1122,92 @@ mod tests {
assert_eq!(17, c.value(4));
}
+ #[test]
+ fn test_date32_month_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(
+ NaiveDate::from_ymd(2000, 1, 1),
+ )]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::new(1, 2)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(
+ c.value(0),
+ Date32Type::from_naive_date(NaiveDate::from_ymd(2001, 3, 1))
+ );
+ }
+
+ #[test]
+ fn test_date32_day_time_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(
+ NaiveDate::from_ymd(2000, 1, 1),
+ )]);
+ let b = IntervalDayTimeArray::from(vec![IntervalDayTimeType::new(1, 2)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(
+ c.value(0),
+ Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 1, 2))
+ );
+ }
+
+ #[test]
+ fn test_date32_month_day_nano_add() {
+ let a = Date32Array::from(vec![Date32Type::from_naive_date(
+ NaiveDate::from_ymd(2000, 1, 1),
+ )]);
+ let b =
+ IntervalMonthDayNanoArray::from(vec![IntervalMonthDayNanoType::new(1, 2, 3)]);
+ let c = add_dyn(&a, &b).unwrap();
+ let c = c.as_any().downcast_ref::<Date32Array>().unwrap();
+ assert_eq!(
+ c.value(0),
+ Date32Type::from_naive_date(NaiveDate::from_ymd(2000, 2, 3))
+ );
+ }
+
+ #[test]
+ fn test_date64_month_add() {
+ let a = Date64Array::from(vec![Date64Type::from_naive_date(
+ NaiveDate::from_ymd(2000, 1, 1),
+ )]);
+ let b = IntervalYearMonthArray::from(vec![IntervalYearMonthType::new(1, 2)]);
+ let c = add_dyn(&a, &b).unwrap();
Review Comment:
it is so great to see `add_dyn` used like this ❤️
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,166 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ /// Creates a IntervalYearMonthType
Review Comment:
I think the functionality for manipulating values on to this `impl` makes sense
Perhaps @viirya was referring to the rust convention that `Type::new()` returns an instance of `Type` -- perhaps if we renamed this method `make_value` or something like that it would be less surprising for other rust developers.
We could do it as a follow on PR as well (before releasing arrow 19.x) too
--
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-rs] alamb commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1212960012
@avantgardnerio I wonder if you are tracking the "add support for adding interval columns to dates/timestamps" in datafusion somewhere?
I ask because https://github.com/apache/arrow-datafusion/pull/3110 by @JasonLi-cn is starting to add support for timestamps
--
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-rs] ursabot commented on pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#issuecomment-1185657669
Benchmark runs are scheduled for baseline = 9d8f0c934a27cd57d36ab4224f232ddf6addf03d and contender = cb7e5b0c8437c3f2ce2ae21951331599de4ccc7d. cb7e5b0c8437c3f2ce2ae21951331599de4ccc7d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/faed54b7870c486f9dac73d8a614d490...0bf5089daa744c89a4bd911cb57f2ebb/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/23342048029645caad84894e084ec9cb...5a0b1acc17fb44d8affa912fc2bffdda/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f54b2d8a51bf4c389bb6ed9e23588bf5...eca457a1a2b54906acd5f189bccca99a/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8147ff1f02474f6095b67dfb9c0f2e2a...ffc5759191904bcdb852c7a6af434312/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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-rs] viirya commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917145705
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -774,6 +778,54 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
DataType::Dictionary(_, _) => {
typed_dict_math_op!(left, right, |a, b| a + b, math_op_dict)
}
+ DataType::Date32 => {
+ let l = as_primitive_array::<Date32Type>(left);
+ match right.data_type() {
+ DataType::Interval(IntervalUnit::YearMonth) => {
+ let r = as_primitive_array::<IntervalYearMonthType>(right);
+ let res = math_op(l, r, Date32Type::add_year_months)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::DayTime) => {
+ let r = as_primitive_array::<IntervalDayTimeType>(right);
+ let res = math_op(l, r, Date32Type::add_day_time)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::MonthDayNano) => {
+ let r = as_primitive_array::<IntervalMonthDayNanoType>(right);
+ let res = math_op(l, r, Date32Type::add_month_day_nano)?;
+ Ok(Arc::new(res))
+ }
+ t => Err(ArrowError::CastError(format!(
+ "Cannot perform arithmetic operation on arrays of type {}",
+ t
+ ))),
+ }
+ }
+ DataType::Date64 => {
+ let l = as_primitive_array::<Date64Type>(left);
+ match right.data_type() {
+ DataType::Interval(IntervalUnit::YearMonth) => {
+ let r = as_primitive_array::<IntervalYearMonthType>(right);
+ let res = math_op(l, r, Date64Type::add_year_months)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::DayTime) => {
+ let r = as_primitive_array::<IntervalDayTimeType>(right);
+ let res = math_op(l, r, Date64Type::add_day_time)?;
+ Ok(Arc::new(res))
+ }
+ DataType::Interval(IntervalUnit::MonthDayNano) => {
+ let r = as_primitive_array::<IntervalMonthDayNanoType>(right);
+ let res = math_op(l, r, Date64Type::add_month_day_nano)?;
+ Ok(Arc::new(res))
+ }
+ t => Err(ArrowError::CastError(format!(
+ "Cannot perform arithmetic operation on arrays of type {}",
+ t
+ ))),
Review Comment:
ditto
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917283995
##########
arrow/src/datatypes/delta.rs:
##########
@@ -0,0 +1,182 @@
+// MIT License
+//
+// Copyright (c) 2020-2022 Oliver Margetts
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+// Copied from chronoutil crate
Review Comment:
The Chrono folks re-opened my PR, so I don't think this will be a concern for too long - it looks like they are interested in building a `add_months()` function into chrono, we're just debating where and how.
--
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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917109375
##########
arrow/Cargo.toml:
##########
@@ -51,8 +51,9 @@ csv_crate = { version = "1.1", default-features = false, optional = true, packag
regex = { version = "1.5.6", default-features = false, features = ["std", "unicode"] }
lazy_static = { version = "1.4", default-features = false }
packed_simd = { version = "0.3", default-features = false, optional = true, package = "packed_simd_2" }
-chrono = { version = "0.4", default-features = false, features = ["clock"] }
+chrono = { version = "0.4", default-features = false, features = ["std", "clock"] }
Review Comment:
This is on longer needed, so I restored it to the version without `std`. Thanks for catching 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-rs] avantgardnerio commented on a diff in pull request #2031: Add support for adding intervals to dates
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #2031:
URL: https://github.com/apache/arrow-rs/pull/2031#discussion_r917120271
##########
arrow/src/datatypes/types.rs:
##########
@@ -191,3 +194,147 @@ impl ArrowTimestampType for TimestampNanosecondType {
TimeUnit::Nanosecond
}
}
+
+impl IntervalYearMonthType {
+ pub fn from(
Review Comment:
I just tried:
```
impl From<(i32, i32, i32> for <IntervalMonthDayNanoType as ArrowPrimitiveType>::Native {
fn from(_: (i32, i32, i32)) -> Self {
todo!()
}
}
```
as a test. I got 902 compile errors, I think the root cause is https://doc.rust-lang.org/error-index.html#E0117
Rust doesn't like it when you try to extend `i32`.
--
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