nrc commented on code in PR #12514:
URL: https://github.com/apache/datafusion/pull/12514#discussion_r1767512808
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -223,9 +239,17 @@ fn seconds(array: &dyn Array, unit: TimeUnit) ->
Result<ArrayRef> {
let secs = as_int32_array(secs.as_ref())?;
let subsecs = date_part(array, DatePart::Nanosecond)?;
let subsecs = as_int32_array(subsecs.as_ref())?;
+ // REVIEW: is there a better way to do this? (Is it even a reasonable
thing to do?) I want to treat
+ // null as 0, I was expecting some kind of map function (or even better
map_if_null) on PrimitiveArray.
+ // Instead I have just discarded the null mask, which feels bad because if
I was meant to be able
+ // to do this, I'd expect there to be a function to do it. I think it is
also possible that a
+ // null-masked value in the array will not be 0 (I don't think that
happens right now, but I think
+ // we're one optimisation away from disaster).
+ let subsecs: PrimitiveArray<Int32Type> =
+ PrimitiveArray::new(subsecs.values().clone(), None);
- let r: Float64Array = binary(secs, subsecs, |secs, subsecs| {
- (secs as f64 + (subsecs as f64 / 1_000_000_000_f64)) * sf
+ let r: Float64Array = binary(secs, &subsecs, |secs, subsecs| {
+ (secs as f64 + ((subsecs % 1_000_000_000) as f64 / 1_000_000_000_f64))
* sf
Review Comment:
Thanks, I'll try that
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
