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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
