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/08/01 20:52:33 UTC
[GitHub] [arrow-rs] alamb commented on a diff in pull request #2251: feat: Implement string cast operations for Time32 and Time64
alamb commented on code in PR #2251:
URL: https://github.com/apache/arrow-rs/pull/2251#discussion_r934903273
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ let sec = time.num_seconds_from_midnight();
+ let frac = time.nanosecond();
+ let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
Review Comment:
It took me a while to grok this -- I think it is leap second handling
https://docs.rs/chrono/0.4.19/chrono/trait.Timelike.html#tymethod.nanosecond
Maybe we could add a comment explaining what was going on
```suggestion
// handle leap second
// see https://docs.rs/chrono/0.4.19/chrono/trait.Timelike.html#tymethod.nanosecond
let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
```
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
Review Comment:
https://github.com/apache/arrow-rs/blob/master/arrow/src/temporal_conversions.rs may be a good place to put these functions too (so they have a chance of being found / reused)
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -136,9 +137,25 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
(Utf8, LargeUtf8) => true,
(LargeUtf8, Utf8) => true,
- (Utf8, Date32 | Date64 | Timestamp(TimeUnit::Nanosecond, None)) => true,
+ (Utf8,
+ Date32
+ | Date64
+ | Time32(TimeUnit::Second)
Review Comment:
❤️
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ let sec = time.num_seconds_from_midnight();
+ let frac = time.nanosecond();
+ let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
+ (sec + adjust) as i32
+}
+
+fn milliseconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ /// The number of nanoseconds per millisecond.
+ const NANOS_PER_MILLI: u32 = 1_000_000;
+ /// The number of milliseconds per second.
+ const MILLIS_PER_SEC: u32 = 1_000;
+
+ let sec = time.num_seconds_from_midnight() * MILLIS_PER_SEC;
+ let frac = time.nanosecond();
+ let (frac, adjust) = if frac < 1_000_000_000 {
+ (frac, 0)
+ } else {
+ (frac - 1_000_000_000, MILLIS_PER_SEC)
+ };
+ (sec + adjust + frac / NANOS_PER_MILLI) as i32
Review Comment:
There is probably a good reason, but my feeble mind can't figure it out. Why is it important to break up `frac` and `adjust`?
For example, isn't this equivalent?
```suggestion
(sec + frac / NANOS_PER_MILLI) as i32
```
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ let sec = time.num_seconds_from_midnight();
+ let frac = time.nanosecond();
+ let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
+ (sec + adjust) as i32
+}
+
+fn milliseconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ /// The number of nanoseconds per millisecond.
+ const NANOS_PER_MILLI: u32 = 1_000_000;
+ /// The number of milliseconds per second.
+ const MILLIS_PER_SEC: u32 = 1_000;
+
+ let sec = time.num_seconds_from_midnight() * MILLIS_PER_SEC;
+ let frac = time.nanosecond();
+ let (frac, adjust) = if frac < 1_000_000_000 {
+ (frac, 0)
+ } else {
+ (frac - 1_000_000_000, MILLIS_PER_SEC)
+ };
+ (sec + adjust + frac / NANOS_PER_MILLI) as i32
+}
+
+/// Casts generic string arrays to `Time32SecondArray`
+fn cast_string_to_time32second<Offset: OffsetSizeTrait>(
+ array: &dyn Array,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<Offset>>()
+ .unwrap();
+
+ let array = if cast_options.safe {
+ let iter = (0..string_array.len()).map(|i| {
+ if string_array.is_null(i) {
+ None
+ } else {
+ string_array
+ .value(i)
+ .parse::<chrono::NaiveTime>()
+ .map(|time| seconds_since_midnight(&time))
+ .ok()
+ }
+ });
+
+ // Benefit:
+ // 20% performance improvement
+ // Soundness:
+ // The iterator is trustedLen because it comes from an `StringArray`.
+ unsafe { Time32SecondArray::from_trusted_len_iter(iter) }
+ } else {
+ let vec = (0..string_array.len())
+ .map(|i| {
+ if string_array.is_null(i) {
+ Ok(None)
+ } else {
+ let string = string_array
+ .value(i);
+ chrono::Duration::days(3);
Review Comment:
The
```
chrono::Duration::days(3);
```
Seems like a leftover?
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ let sec = time.num_seconds_from_midnight();
+ let frac = time.nanosecond();
+ let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
+ (sec + adjust) as i32
+}
+
+fn milliseconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ /// The number of nanoseconds per millisecond.
+ const NANOS_PER_MILLI: u32 = 1_000_000;
+ /// The number of milliseconds per second.
+ const MILLIS_PER_SEC: u32 = 1_000;
+
+ let sec = time.num_seconds_from_midnight() * MILLIS_PER_SEC;
+ let frac = time.nanosecond();
+ let (frac, adjust) = if frac < 1_000_000_000 {
+ (frac, 0)
+ } else {
+ (frac - 1_000_000_000, MILLIS_PER_SEC)
+ };
+ (sec + adjust + frac / NANOS_PER_MILLI) as i32
+}
+
+/// Casts generic string arrays to `Time32SecondArray`
+fn cast_string_to_time32second<Offset: OffsetSizeTrait>(
+ array: &dyn Array,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<Offset>>()
+ .unwrap();
+
+ let array = if cast_options.safe {
+ let iter = (0..string_array.len()).map(|i| {
+ if string_array.is_null(i) {
+ None
+ } else {
+ string_array
+ .value(i)
+ .parse::<chrono::NaiveTime>()
+ .map(|time| seconds_since_midnight(&time))
+ .ok()
+ }
+ });
Review Comment:
I think the same type of transformation can be applied to the iterator below this as well.
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1584,6 +1625,303 @@ fn cast_string_to_date64<Offset: OffsetSizeTrait>(
Ok(Arc::new(array) as ArrayRef)
}
+fn seconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ let sec = time.num_seconds_from_midnight();
+ let frac = time.nanosecond();
+ let adjust = if frac < 1_000_000_000 { 0 } else { 1 };
+ (sec + adjust) as i32
+}
+
+fn milliseconds_since_midnight(time: &chrono::NaiveTime) -> i32 {
+ /// The number of nanoseconds per millisecond.
+ const NANOS_PER_MILLI: u32 = 1_000_000;
+ /// The number of milliseconds per second.
+ const MILLIS_PER_SEC: u32 = 1_000;
+
+ let sec = time.num_seconds_from_midnight() * MILLIS_PER_SEC;
+ let frac = time.nanosecond();
+ let (frac, adjust) = if frac < 1_000_000_000 {
+ (frac, 0)
+ } else {
+ (frac - 1_000_000_000, MILLIS_PER_SEC)
+ };
+ (sec + adjust + frac / NANOS_PER_MILLI) as i32
+}
+
+/// Casts generic string arrays to `Time32SecondArray`
+fn cast_string_to_time32second<Offset: OffsetSizeTrait>(
+ array: &dyn Array,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef> {
+ let string_array = array
+ .as_any()
+ .downcast_ref::<GenericStringArray<Offset>>()
+ .unwrap();
+
+ let array = if cast_options.safe {
+ let iter = (0..string_array.len()).map(|i| {
+ if string_array.is_null(i) {
+ None
+ } else {
+ string_array
+ .value(i)
+ .parse::<chrono::NaiveTime>()
+ .map(|time| seconds_since_midnight(&time))
+ .ok()
+ }
+ });
Review Comment:
I think this will be faster as it will not require checking bounds for calls to `is_null` or `value`:
```suggestion
let iter = string_array
.iter()
.flat_map(|v| {
v.map(|v| {
v.parse::<chrono::NaiveTime>()
.map(|time| seconds_since_midnight(&time))
.ok()
})
});
```
--
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