neilconway commented on code in PR #11187: URL: https://github.com/apache/arrow-rs/pull/11187#discussion_r4098390293
########## arrow-arith/src/temporal.rs: ########## @@ -184,6 +184,10 @@ where /// Returns an [`Int32Array`] unless input was a dictionary type, in which case returns /// the dictionary but with this function applied onto its values. /// +/// Null inputs produce null outputs. A timestamp outside the supported calendar +/// range also produces null, except that for timestamps without a timezone the +/// time parts (`Hour` through `Nanosecond`) are defined for every value. Review Comment: My two cents: the current behavior implemented by the PR is the better option. The historical behavior of `date_part` is an unfortunate implementation detail from using Chrono; defining `date_part` for a wider range is values is not inherently bad (it's good, actually!), and it is also considerably faster. The main downside is inconsistency between extracting different time components for timestamps with extreme values. That's a bit odd but it doesn't seem like a showstopper to me. The current behavior for out-of-range timestamps is arguably a defect to begin with -- e.g., in the future we could perhaps improve `date_part` so that it is defined for all timestamps and all time components. -- 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]
