-
Notifications
You must be signed in to change notification settings - Fork 716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust Interval definition incorrect #5654
Comments
@pitrou do you know if the integration tests cover the interval types? @avantgardnerio as the author of #2235 which appears to have switched these round, perhaps you could weigh in here |
Honestly, I don't recall the reasoning. I know at the time I was working with JDBC interop, so I assume it was to make that work. I agree we should do whatever arrow-cpp does, as that seems to be the reference implementation, to the extent that there is one. |
|
One way to avoid this leading to subtle downstream breakage might be to do something along the lines of #3125 |
Typing down some things that @tustvold told me today:
|
@tustvold reports is that it is unlikely that this will be done this week due to other higher priorities |
It turns out the reason we didn't detect this with our integration tests, is that they were actually using different, correct, logic for constructing the expected values from the JSON data - #5765 |
@alamb and I had a good discussion about the pros/cons of switching to a structured representation (#3125), vs simply flipping the order (#5656). The broad points were:
As such the decision was that a custom struct should be used where operations, such as arithmetic, on the integral representation have no semantic meaning. Specifically this means types such as timestamps, decimals, and even IntervalYearMonth should remain using integral types as their native representation, as operations on the integral representation are well-defined. However, types like IntervalMonthDayNano or ByteView (#5736) should switch to a non-integral representation. |
That's great to know. Any idea on the ETA on this fix? |
We are holding the |
Proposed PR: #5769 |
|
|
It seems the Rust
INTERVAL
definition is incorrect. Please see duckdb/duckdb-wasm#1696 (comment) for more details on the bug and where it occurs in the code.Particularly this definition:
arrow-rs/arrow-array/src/types.rs
Lines 265 to 269 in 36a6e51
The text was updated successfully, but these errors were encountered: