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/11/13 10:41:28 UTC
[GitHub] [arrow-rs] Jefffrey opened a new pull request, #3101: Parse Time32/Time64 from formatted string
Jefffrey opened a new pull request, #3101:
URL: https://github.com/apache/arrow-rs/pull/3101
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3100.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
[GitHub] [arrow-rs] ursabot commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1315929189
Benchmark runs are scheduled for baseline = c99d2f333f9c1acda1aeaa646b04405c3eae5044 and contender = c95eb4c80a532653bc91e04e78814f1282c8d005. c95eb4c80a532653bc91e04e78814f1282c8d005 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/ca4099d5d2f344fabbe5ec60f8b7a18d...b388fb21598241b4adff106881f89e63/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/583abad6c009486fa6afb74742e944c3...df31b31f0e964335bf5c994edec4829a/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c642c9831c454b0fa200d993a34f8da1...1fccdd2d6f7443bc963190b8dcbce078/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/fc0af0edf87642558630a6ffd8cb8199...ad83a5afe04144fd8d8ff22db8eed7c9/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow-rs] waitingkuo commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
waitingkuo commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022272744
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
Review Comment:
I'll suggest that move this as the first place as this is our default display (`23:59:59.123456789`)
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
+ "%k:%M:%S%.9f",
+ "%I:%M:%S %P",
+ "%I:%M:%S %p",
+ "%l:%M:%S %P",
+ "%l:%M:%S %p",
+ "%H:%M:%S",
+ "%k:%M:%S",
+ "%I:%M %P",
+ "%I:%M %p",
+ "%l:%M %P",
+ "%l:%M %p",
+ "%H:%M",
+ "%k:%M",
+ ]
+ .iter()
+ .find_map(|f| NaiveTime::parse_from_str(string, f).ok())
+ .map(|nt| {
+ nt.num_seconds_from_midnight() as i64 * 1_000_000_000 + nt.nanosecond() as i64
+ })
+ .or_else(|| string.parse::<Self::Native>().ok())
+ }
+}
+
+impl Parser for Time64MicrosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.6f %P",
+ "%I:%M:%S%.6f %p",
+ "%l:%M:%S%.6f %P",
+ "%l:%M:%S%.6f %p",
+ "%H:%M:%S%.6f",
+ "%k:%M:%S%.6f",
+ "%I:%M:%S %P",
+ "%I:%M:%S %p",
+ "%l:%M:%S %P",
+ "%l:%M:%S %p",
+ "%H:%M:%S",
+ "%k:%M:%S",
+ "%I:%M %P",
+ "%I:%M %p",
+ "%l:%M %P",
+ "%l:%M %p",
+ "%H:%M",
+ "%k:%M",
+ ]
+ .iter()
+ .find_map(|f| NaiveTime::parse_from_str(string, f).ok())
+ .map(|nt| {
+ nt.num_seconds_from_midnight() as i64 * 1_000_000
+ + (nt.nanosecond() as i64) / 1_000
+ })
Review Comment:
we could consider whether to support leap second `23:59:60`
`%S` already captured `Second number (00–60), zero-padded to 2 digits.
`22:59:60` is parsed as `82800000000000 nanos` which works.
`23:59:60` is parsed as `86400000000000 nanos` which overflows while we construct the array by `Time64NanosecondArray::from(vec![86400000000000]);` so it'll return a `null`
--
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
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022555512
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
Review Comment:
Implemented the parse_formatted function, and also implemented a preprocess on the string to prune the formats to attempt parsing for
--
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
[GitHub] [arrow-rs] Jefffrey commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1313120691
It's a fair point, as I was unsure which to support too. I was planning to base it on other arrow implementations, like pyarrow, but had difficulty tracking down the actual code that did the parsing, to reference, so I just went and did a bunch of formats which seemed reasonable. Happy to cut down any if its too broad
--
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
[GitHub] [arrow-rs] Jefffrey commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1315045458
A behaviour I've changed which is worth noting is that it is valid to pass in fractions of a second, even if the type you're parsing for doesn't support that precision; it'll simply be truncated from the final representation.
See:
```rust
assert_eq!(Time32SecondType::parse("02:10:01.1"), Some(7_801));
```
This technically was already happening for milli/micro/nano seconds anyway, but has been extended to seconds as well, to centralize all the behaviour. Let me know any thoughts on if instead it should be stricter and fail the parsing, rather than passing and truncating.
--
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
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022404078
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
Review Comment:
Sure thing, will do
--
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
[GitHub] [arrow-rs] Jefffrey commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1313327223
> One other option would be to support a default representation, but support specifying a custom format string by using `Parse::parse_formatted` originally added by @sum12 in #1451
Can definitely add the support for `Parse::parse_formatted`, but still would leave the question of what the default(s) should be, especially for the original use case of parsing from csv. Ideally wouldn't want to have to slap `parse_formatted` for all the different representations in here:
https://github.com/apache/arrow-rs/blob/3ca41f50d0e8b6da95d83e5bf0b09fd518e2110f/arrow-csv/src/reader.rs#L587-L604
--
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
[GitHub] [arrow-rs] waitingkuo commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
waitingkuo commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022284434
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
Review Comment:
this one `%H:%M:%S%.9f`
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1313125388
One other option would be to support a default representation, but support specifying a custom format string by using `Parse::parse_formatted` originally added by @sum12 in https://github.com/apache/arrow-rs/pull/1451
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022084444
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
Review Comment:
One other option to speed this up might be to do a pass through the string and compute
* Number of colons
* Presence of space
* Presence of capital `M`
* Presence of decimal point
And use this to prune the list of candidates
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1023191009
##########
arrow-cast/src/parse.rs:
##########
@@ -132,6 +132,106 @@ pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
)))
}
+/// Accepts a string in ISO8601 standard format and some
+/// variants and converts it to nanoseconds since midnight.
+///
+/// Examples of accepted inputs:
+/// * `09:26:56.123 AM`
+/// * `23:59:59`
+/// * `6:00 pm`
+//
+/// Internally, this function uses the `chrono` library for the
+/// time parsing
+///
+/// ## Timezone / Offset Handling
+///
+/// This function does not support parsing strings with a timezone
+/// or offset specified, as it considers only time since midnight.
+pub fn string_to_time_nanoseconds(s: &str) -> Result<i64, ArrowError> {
+ // colon count, presence of decimal, presence of whitespace
+ fn preprocess_time_string(string: &str) -> (u8, bool, bool) {
+ string
+ .chars()
+ .fold((0, false, false), |tup, char| match char {
+ ':' => (tup.0.saturating_add(1), tup.1, tup.2),
Review Comment:
```suggestion
.fold((0_usize, false, false), |tup, char| match char {
':' => (tup.0 + 1, tup.1, tup.2),
```
Using a usize means this can't actually overflow
##########
arrow-cast/src/parse.rs:
##########
@@ -411,4 +577,265 @@ mod tests {
parse_timestamp("2020-09-08 13:42:29").unwrap()
);
}
+
+ #[test]
+ fn parse_time64_nanos() {
+ assert_eq!(
+ Time64NanosecondType::parse("02:10:01.1234567899999999"),
+ Some(7_801_123_456_789)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("02:10:01.1234567"),
+ Some(7_801_123_456_700)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01.1234567"),
+ Some(7_801_123_456_700)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("12:10:01.123456789 AM"),
+ Some(601_123_456_789)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("12:10:01.123456789 am"),
+ Some(601_123_456_789)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01.12345678 PM"),
+ Some(51_001_123_456_780)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01.12345678 pm"),
+ Some(51_001_123_456_780)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("02:10:01"),
+ Some(7_801_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01"),
+ Some(7_801_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("12:10:01 AM"),
+ Some(601_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("12:10:01 am"),
+ Some(601_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01 PM"),
+ Some(51_001_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10:01 pm"),
+ Some(51_001_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("02:10"),
+ Some(7_800_000_000_000)
+ );
+ assert_eq!(Time64NanosecondType::parse("2:10"), Some(7_800_000_000_000));
+ assert_eq!(
+ Time64NanosecondType::parse("12:10 AM"),
+ Some(600_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("12:10 am"),
+ Some(600_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10 PM"),
+ Some(51_000_000_000_000)
+ );
+ assert_eq!(
+ Time64NanosecondType::parse("2:10 pm"),
+ Some(51_000_000_000_000)
+ );
+
+ // parse directly as nanoseconds
+ assert_eq!(Time64NanosecondType::parse("1"), Some(1));
+
+ // colon overflow
+ assert_eq!(
+ Time64NanosecondType::parse(&(0..=256).map(|_| ':').collect::<String>()),
+ None
+ );
+
+ // leap second
+ assert_eq!(
+ Time64NanosecondType::parse("23:59:60"),
+ Some(86_400_000_000_000)
+ );
+
+ // custom format
Review Comment:
:+1:
##########
arrow-cast/src/parse.rs:
##########
@@ -132,6 +132,106 @@ pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
)))
}
+/// Accepts a string in ISO8601 standard format and some
+/// variants and converts it to nanoseconds since midnight.
+///
+/// Examples of accepted inputs:
+/// * `09:26:56.123 AM`
+/// * `23:59:59`
+/// * `6:00 pm`
+//
+/// Internally, this function uses the `chrono` library for the
+/// time parsing
+///
+/// ## Timezone / Offset Handling
+///
+/// This function does not support parsing strings with a timezone
+/// or offset specified, as it considers only time since midnight.
+pub fn string_to_time_nanoseconds(s: &str) -> Result<i64, ArrowError> {
+ // colon count, presence of decimal, presence of whitespace
+ fn preprocess_time_string(string: &str) -> (u8, bool, bool) {
+ string
+ .chars()
Review Comment:
```suggestion
.as_bytes()
.iter()
```
And then match using `b':'`
We don't actually need to use `chars` here, as the nature of the UTF-8 encoding is such that ASCII can be compared without ambiguity - https://en.wikipedia.org/wiki/UTF-8#Encoding
##########
arrow-cast/src/parse.rs:
##########
@@ -132,6 +132,106 @@ pub fn string_to_timestamp_nanos(s: &str) -> Result<i64, ArrowError> {
)))
}
+/// Accepts a string in ISO8601 standard format and some
+/// variants and converts it to nanoseconds since midnight.
+///
+/// Examples of accepted inputs:
+/// * `09:26:56.123 AM`
+/// * `23:59:59`
+/// * `6:00 pm`
+//
+/// Internally, this function uses the `chrono` library for the
+/// time parsing
+///
+/// ## Timezone / Offset Handling
+///
+/// This function does not support parsing strings with a timezone
+/// or offset specified, as it considers only time since midnight.
+pub fn string_to_time_nanoseconds(s: &str) -> Result<i64, ArrowError> {
+ // colon count, presence of decimal, presence of whitespace
+ fn preprocess_time_string(string: &str) -> (u8, bool, bool) {
+ string
+ .chars()
+ .fold((0, false, false), |tup, char| match char {
+ ':' => (tup.0.saturating_add(1), tup.1, tup.2),
+ '.' => (tup.0, true, tup.2),
+ ' ' => (tup.0, tup.1, true),
+ _ => tup,
+ })
+ }
+
+ fn naive_time_parser(string: &str, formats: &[&str]) -> Option<NaiveTime> {
Review Comment:
It might be cleaner to make the match block evaluate to `&[&str]` and then have this follow.
e.g. something like
```
let formats = match preprocess_time_string(s.trim()) {
...
};
formats
.iter()
.find_map(|f| NaiveTime::parse_from_str(string, f).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
[GitHub] [arrow-rs] tustvold commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1313115338
This looks good to me, I especially love the test coverage.
Before merging I would like to get consensus on whether we want to support such a broad range of representations, I could definitely see an argument to just support RFC3339 style times, i.e. `%H:%M:%S`.
Thoughts @waitingkuo @alamb ?
--
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
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022560318
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
Review Comment:
Should no longer be relevant as I've implemented a preprocess pass which should ensure there isn't a priority clash between 24 and 12 hour time formats now
--
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
[GitHub] [arrow-rs] tustvold merged pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#issuecomment-1314868053
> 4. consider the timezone
I originally was going to suggest this, but I decided against it as the semantics are actually a little bit funky. In particular, courtesy of the wonders of daylight savings, a non-FixedOffset timezone requires the date in order to be interpreted. I think it is acceptable to only handle timezones for timestamps, and not for times. FWIW this is the same approach taken by chrono - there is `DateTime<Tz>` and `NaiveDateTime` but only `NaiveTime` and no `Time<Tz>`
--
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
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022390861
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
Review Comment:
Thanks for the feedback, I'll update the documentation & take a shot at implementing that string pre-pass to prune the formats to try parse for
--
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
[GitHub] [arrow-rs] Jefffrey commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
Jefffrey commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022554367
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.9f %P",
+ "%I:%M:%S%.9f %p",
+ "%l:%M:%S%.9f %P",
+ "%l:%M:%S%.9f %p",
+ "%H:%M:%S%.9f",
+ "%k:%M:%S%.9f",
+ "%I:%M:%S %P",
+ "%I:%M:%S %p",
+ "%l:%M:%S %P",
+ "%l:%M:%S %p",
+ "%H:%M:%S",
+ "%k:%M:%S",
+ "%I:%M %P",
+ "%I:%M %p",
+ "%l:%M %P",
+ "%l:%M %p",
+ "%H:%M",
+ "%k:%M",
+ ]
+ .iter()
+ .find_map(|f| NaiveTime::parse_from_str(string, f).ok())
+ .map(|nt| {
+ nt.num_seconds_from_midnight() as i64 * 1_000_000_000 + nt.nanosecond() as i64
+ })
+ .or_else(|| string.parse::<Self::Native>().ok())
+ }
+}
+
+impl Parser for Time64MicrosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ [
+ "%I:%M:%S%.6f %P",
+ "%I:%M:%S%.6f %p",
+ "%l:%M:%S%.6f %P",
+ "%l:%M:%S%.6f %p",
+ "%H:%M:%S%.6f",
+ "%k:%M:%S%.6f",
+ "%I:%M:%S %P",
+ "%I:%M:%S %p",
+ "%l:%M:%S %P",
+ "%l:%M:%S %p",
+ "%H:%M:%S",
+ "%k:%M:%S",
+ "%I:%M %P",
+ "%I:%M %p",
+ "%l:%M %P",
+ "%l:%M %p",
+ "%H:%M",
+ "%k:%M",
+ ]
+ .iter()
+ .find_map(|f| NaiveTime::parse_from_str(string, f).ok())
+ .map(|nt| {
+ nt.num_seconds_from_midnight() as i64 * 1_000_000
+ + (nt.nanosecond() as i64) / 1_000
+ })
Review Comment:
All four should now support having a leap second, per what chrono NaiveTime also supports
--
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
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3101: Parse Time32/Time64 from formatted string
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3101:
URL: https://github.com/apache/arrow-rs/pull/3101#discussion_r1022037497
##########
arrow-cast/src/parse.rs:
##########
@@ -199,10 +199,123 @@ impl Parser for TimestampSecondType {
}
}
-parser_primitive!(Time64NanosecondType);
-parser_primitive!(Time64MicrosecondType);
-parser_primitive!(Time32MillisecondType);
-parser_primitive!(Time32SecondType);
+impl Parser for Time64NanosecondType {
+ fn parse(string: &str) -> Option<Self::Native> {
Review Comment:
I think accepting a wide range of formats is consistent with `string_to_timestamp_nanos` for better or worse
https://github.com/Jefffrey/arrow-rs/blob/3ca41f50d0e8b6da95d83e5bf0b09fd518e2110f/arrow-cast/src/parse.rs#L71-L133
The only thing I recommend is adding docstring documentation (that will show up on docs.rs) for the types of formats accepted. We could follow the example of string_to_timestamp_nanos :
https://github.com/Jefffrey/arrow-rs/blob/3ca41f50d0e8b6da95d83e5bf0b09fd518e2110f/arrow-cast/src/parse.rs#L23-L54
--
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