Rust Interval definition incorrect

[david542542]

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

	```text
	┌──────────────────────────────┬─────────────┬──────────────┐
	│ Nanos │ Days │ Months │
	│ (64 bits) │ (32 bits) │ (32 bits) │
	└──────────────────────────────┴─────────────┴──────────────┘

[tustvold]

@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

[avantgardnerio]

@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.

[pitrou]

@pitrou do you know if the integration tests cover the interval types?

Yes, they do.
https://github.com/apache/arrow/blob/7003e90c113cec58a620deddf56af71eb305af2a/dev/archery/archery/integration/datagen.py#L1653-L1669

[tustvold]

One way to avoid this leading to subtle downstream breakage might be to do something along the lines of #3125

[alamb]

Typing down some things that @tustvold told me today:

1. we think the tests ensure the data round trips but does not actually ensure both ends interpret them in the same way (aka they don't cover this particular bug)
2. He plans to work on this ticket, likely next week(ish)
3. In order to avoid subtle breaking changes (due to the same type i128 being interpreted differently by different versions) he plans to do something like Structured Interval Native Type #3125

[alamb]

@tustvold reports is that it is unlikely that this will be done this week due to other higher priorities

[tustvold]

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

[tustvold]

@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:

• There are extremely few, if any, operations that are well defined for the integral representation of compound types
• Switching to a structured type would allow us to keep the sort order the same
• Switching to a structured type would make it easy to find code that is making incorrect layout assumptions
• Arrow-cpp uses a structured type - https://github.com/apache/arrow/blob/433ceef8a292932b31ddb3127f95eeb85ae41df5/cpp/src/arrow/type.h#L1731

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.

[david542542]

That's great to know. Any idea on the ETA on this fix?

[alamb]

We are holding the 52.0.0 release for this. I expect it will be available in about 2 weeks but no promises, etc

[alamb]

Proposed PR: #5769

[david542542]

@alamb any idea on when 52.0.0 will be deployed?

[alamb]

@alamb any idea on when 52.0.0 will be deployed?

The plan as I understand it is next week

[tustvold]

label_issue.py automatically added labels {'parquet'} from #5769

[tustvold]

label_issue.py automatically added labels {'arrow'} from #5769