tustvold commented on code in PR #5542:
URL: https://github.com/apache/arrow-rs/pull/5542#discussion_r1536584842
##########
arrow-cast/src/parse.rs:
##########
@@ -171,25 +181,30 @@ impl TimestampParser {
///
/// [IANA timezones]: https://www.iana.org/time-zones
pub fn string_to_datetime<T: TimeZone>(timezone: &T, s: &str) ->
Result<DateTime<T>, ArrowError> {
+ #[allow(deprecated)]
Review Comment:
I'm pretty lukewarm on this as I understand chrono intends to remove these
methods in future. Is there a NaiveTime::MIN or something we could use?
##########
arrow-cast/src/parse.rs:
##########
@@ -24,9 +24,18 @@ use chrono::prelude::*;
use half::f16;
use std::str::FromStr;
+/// Parse nanoseconds from the first `N` values in digits
+#[inline]
+fn parse_nanos<const N: usize>(digits: &[u8]) -> u32 {
+ digits[..N]
+ .iter()
+ .fold(0_u32, |acc, v| acc * 10 + *v as u32)
+ * 10_u32.pow((9 - N) as _)
+}
+
/// Parse nanoseconds from the first `N` values in digits, subtracting the
offset `O`
#[inline]
-fn parse_nanos<const N: usize, const O: u8>(digits: &[u8]) -> u32 {
+fn parse_nanos_subtracting<const N: usize, const O: u8>(digits: &[u8]) -> u32 {
Review Comment:
Have you benchmarked to confirm that this separation makes a difference to
performance, I would be extremely surprised if these don't compile to the same
thing
##########
arrow-cast/src/parse.rs:
##########
@@ -171,25 +181,30 @@ impl TimestampParser {
///
/// [IANA timezones]: https://www.iana.org/time-zones
pub fn string_to_datetime<T: TimeZone>(timezone: &T, s: &str) ->
Result<DateTime<T>, ArrowError> {
+ #[allow(deprecated)]
+ const ZERO_TIME: NaiveTime = NaiveTime::from_hms(0, 0, 0);
+
let err =
|ctx: &str| ArrowError::ParseError(format!("Error parsing timestamp
from '{s}': {ctx}"));
let bytes = s.as_bytes();
- if bytes.len() < 10 {
+ let len = bytes.len();
Review Comment:
Again I am very surprised if this makes any performance difference,
compilers are smart enough to figure this out
##########
arrow-array/src/timezone.rs:
##########
@@ -25,27 +25,43 @@ pub use private::{Tz, TzOffset};
fn parse_fixed_offset(tz: &str) -> Option<FixedOffset> {
Review Comment:
My benchmarks are not showing this code as being made any faster (or slower)
--
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]
