ryux1 commented on code in PR #11006:
URL: https://github.com/apache/arrow-rs/pull/11006#discussion_r3944308623
##########
arrow-cast/src/parse.rs:
##########
@@ -1653,7 +1653,19 @@ fn parse_interval_components(
// parse amounts and units
let Ok(pairs): Result<Vec<(IntervalAmount, IntervalUnit)>, ArrowError> =
raw_pairs
.iter()
- .map(|(a, u)| Ok((a.parse()?, IntervalUnit::from_str_or_config(*u,
config)?)))
+ .map(|(a, u)| {
+ // A unit with no preceding amount (e.g. the "hour" in "5 day
hour")
+ // is treated as an amount of zero, matching PostgreSQL.
+ let amount = if a.is_empty() {
Review Comment:
This also changes the empty string: `split_interval_components("")` returns
`[("", None)]`, so this branch now turns `Interval::parse("", &config)` from an
error into zero in the configured default unit. Unlike `"hour"`, that input
contains no bare unit. Is accepting empty intervals intentional? If not, could
the zero special case require `u.is_some()` and add a regression asserting `""`
remains invalid? If it is intentional PostgreSQL compatibility, it should be
called out and tested explicitly because it broadens the public parser beyond
#6390's unit-without-amount case.
##########
arrow-cast/src/parse.rs:
##########
@@ -1653,7 +1653,19 @@ fn parse_interval_components(
// parse amounts and units
let Ok(pairs): Result<Vec<(IntervalAmount, IntervalUnit)>, ArrowError> =
raw_pairs
.iter()
- .map(|(a, u)| Ok((a.parse()?, IntervalUnit::from_str_or_config(*u,
config)?)))
+ .map(|(a, u)| {
+ // A unit with no preceding amount (e.g. the "hour" in "5 day
hour")
+ // is treated as an amount of zero, matching PostgreSQL.
+ let amount = if a.is_empty() {
Review Comment:
This also changes the empty string: `split_interval_components("")` returns
`[("", None)]`, so this branch now turns `Interval::parse("", &config)` from an
error into zero in the configured default unit. Unlike `"hour"`, that input
contains no bare unit. Is accepting empty intervals intentional? If not, could
the zero special case require `u.is_some()` and add a regression asserting `""`
remains invalid? If it is intentional PostgreSQL compatibility, it should be
called out and tested explicitly because it broadens the public parser beyond
#6390's unit-without-amount case.
--
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]
