feat(cast): treat bare interval units as zero (#6390) - #11006
Conversation
ryux1
left a comment
There was a problem hiding this comment.
The bare-unit cases are covered clearly. I found one input whose behavior changes beyond that stated scope.
| .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() { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Good catch. I've tightened the change so an empty amount is only treated as zero when a unit is actually present (a.is_empty() && u.is_some()), leaving Interval::parse("") invalid.
Added a regression to test_interval_bare_units asserting "" still errors, and verified that test fails if the u.is_some() guard is removed.
This also means the change is now strictly scoped to #6390's unit-without-amount case: the zero only applies to strings that contain a bare unit.
There was a problem hiding this comment.
That guard and regression cover the scope issue I found. Thanks for tightening it.
ryux1
left a comment
There was a problem hiding this comment.
The bare-unit cases are covered clearly. I found one input whose behavior changes beyond that stated scope.
|
this doesnt seem accepted by either spark or duckdb duckdb: memory D select interval '5 day hour';
Conversion Error:
Could not convert string '5 day hour' to INTERVAL
LINE 1: select interval '5 day hour';spark: Traceback (most recent call last):
File "<python-input-4>", line 1, in <module>
spark.sql("select interval '5 day hour'").show()
~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/jeffrey/.cache/uv/archive-v0/DYpV4hSlk1WPp1Lj/lib/python3.13/site-packages/pyspark/sql/session.py", line 1915, in sql
return DataFrame(self._jsparkSession.sql(sqlQuery, litArgs), self)
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
File "/Users/jeffrey/.cache/uv/archive-v0/DYpV4hSlk1WPp1Lj/lib/python3.13/site-packages/py4j/java_gateway.py", line 1362, in __call__
return_value = get_return_value(
answer, self.gateway_client, self.target_id, self.name)
File "/Users/jeffrey/.cache/uv/archive-v0/DYpV4hSlk1WPp1Lj/lib/python3.13/site-packages/pyspark/errors/exceptions/captured.py", line 257, in deco
raise converted from None
pyspark.errors.exceptions.captured.ParseException:
[INVALID_TYPED_LITERAL] The value of the typed literal "INTERVAL" is invalid: '5 day hour'. SQLSTATE: 42604
== SQL (line 1, position 8) ==
select interval '5 day hour'so im pretty lukewarm on accepting this, especially as it just doesnt read quite well as an interval; it reads more like a mistake from postgres' permissive parsing |
PostgreSQL interprets an interval unit with no preceding amount as an amount of zero, e.g. '5 day hour' is the same as '5 day 0 hour'. The interval parser instead rejected such strings because the empty amount failed to parse. Handle an empty amount in parse_interval_components by using a zero IntervalAmount, but only when a unit is actually present so an empty string remains invalid. Unknown units and repeated units still error. Closes apache#6390
02a7d02 to
ef1fb85
Compare
|
Thanks for checking Spark and DuckDB behavior — good data point. My reasoning for keeping this scoped: the arrow-rs interval parser is explicitly documented to match PostgreSQL's interval parser ( That said, I hear the concern that it "reads like a mistake". I've tightened the scope so the relaxation only applies to a bare unit with a unit present —
Happy to adjust either way. |
|
im of a similar mind to the existing discussion in #6390; if this PR is meant to be for a use case that needs it it would make a stronger case, however given how odd & niche it seems im not sure we should aim to support it 🤔 |
What does this PR do?
PostgreSQL interprets an interval unit with no preceding amount as an amount of zero —
'5 day hour'is the same as'5 day 0 hour'.parse_interval_month_day_nano_config(and friends) instead rejected such strings, because the empty amount failed to parse.This is the gap behind the DataFusion case
SELECT interval '5 day' hour(which becomes the interval string"5 day HOUR"), see apache/datafusion#12448. The arrow-rs interval parser is documented to match PostgreSQL's interval parser (arrow-cast/src/parse.rs,split_interval_componentsdoc comment).Changes
In
parse_interval_components(arrow-cast/src/parse.rs), an empty amount string is now treated as a zeroIntervalAmountonly when a unit is actually present:"5 day hour"→5 days(like"5 day 0 hour")"5 day HOUR"also parses"hour", is zero""contains no unit and remains invalidOne existing test changed:
"1h s"previously errored and now parses as1 hour(the barescontributes zero). This is the intended permissiveness change from #6390.Checklist
cargo test -p arrow-cast(392 passed)""regression test fails if theu.is_some()guard is removedcargo clippy -p arrow-cast --all-targetscargo fmt -p arrow-cast -- --checktypos