feat(isthmus): migrate to PrecisionTime / PrecisionTimestamp

[nielspardon]

This PR migrates isthmus to use PrecisionTime / PrecisionTimestamp instead of the deprecated Time / Timestamp types.

[bestbeforetoday]

Do we still need to test req_ts?

[nielspardon]

Round tripping function calls with the deprecated Timestamp, TimestampTZ, Time was already broken in some cases before this change. We don't have tests that certify backwards compatibility of parsing Substrait plans using deprecated types / functions.

[bestbeforetoday]

This doesn't look to be used anywhere in the codebase. If it is worth adding, it should probably have a test driving it.

[bestbeforetoday]

This doesn't look to be used anywhere in the codebase. If it is worth adding, it should probably have a test driving it.

[bestbeforetoday]

Thinking more on the new ExpressionCreator precisionTime and precisionTimestamp functions, it seems that these both create expressions that cannot then be converted to SQL since they exceed the maximum precision supported by Calcite and so will result in failure (in Isthmus) due to the precision checks this PR also adds. Assuming I have that correct, are we implementing the most useful behaviour? Should the precisionTime and precisionTimestamp functions create expressions at a precision that can be successfully converted to Calcite, or should we be more lenient on the conversion and truncate (or round) values during conversion to Calcite instead of failing? Maybe it is still fine since you might just be creating a Substrait plan that is not intended to be converted to Calcite using Isthmus? Worth some consideration anyway.

[bestbeforetoday]

I see the build failed because some test was expecting UnsupportedOperationException instead of IllegalArgumentException. If that is existing behaviour then it seems OK to me to preserve it, even if it also seems an unintuitive and potentially confusing exception type to me. Your choice.

[nielspardon]

I see the build failed because some test was expecting UnsupportedOperationException instead of IllegalArgumentException. If that is existing behaviour then it seems OK to me to preserve it, even if it also seems an unintuitive and potentially confusing exception type to me. Your choice.

I fixed all occurences. We had the same pattern also in another class.

[nielspardon]

Assuming I have that correct, are we implementing the most useful behaviour? Should the precisionTime and precisionTimestamp functions create expressions at a precision that can be successfully converted to Calcite, or should we be more lenient on the conversion and truncate (or round) values during conversion to Calcite instead of failing?

I think this a complex and separate topic.

• ExpressionCreator is a component in core which means it can be used independently of isthmus / Calcite. - However the existing LocalDateTime, LocalDate, LocalTime method variants in the creator were originally created to make the interoperability with Calcite easier since you can get one of the above mentioned objects from a Calcite datetime expression.
• I thought it might in general (independently of Calcite) be useful to create Substrait datetime objects from Java datetime objects without consumers of the substrait-java core having to do the conversion to milli, micro, nano, pico seconds themselves
• How we handle compatibility between core / isthmus is yet another topic. We may even need to handle precision compatibility on the SQL dialect level. I expect the Substrait dialect to play a role here. I recently added support for specifying the maximum supported precision for datetime types to the Substrait dialect: feat(dialect): support specifying maximum supported subsecond precision substrait#961