feat: precision literal implementations#178
Conversation
wackywendell
left a comment
wackywendell
left a comment
There was a problem hiding this comment.
Overall, this is a good addition: updates GRAMMAR, parser, and textifier, with good round-trip tests!
There's a few places where I think we need to handle out-of-range values; I commented specifically, but generally, we should either support them or error, not silently ignore / truncate / wrap around.
Generally, though, good work, keep it going!
| precision_timestamp_tz_type := "precisiontimestamptz" nullability? "<" integer ">" | ||
| ``` | ||
|
|
||
| The precision parameter follows Substrait's unit convention: `0` means seconds, `3` milliseconds, `6` microseconds, `9` nanoseconds, and `12` picoseconds. Type syntax accepts values from `0` through `12`; precision literal parsing currently supports `0`, `3`, `6`, and `9`. |
There was a problem hiding this comment.
Could we make the supported literal subset more explicit here? Restricting literals to the common precisions 0, 3, 6, and 9 seems reasonable, but it would help to distinguish that from the full Substrait type range of 0..=12. It would also be useful to mention that precision-12 protobuf literals textify as precision 9 with a truncation warning.
| let duration = duration_from_precision_units(value, precision)?; | ||
| let epoch = DateTime::from_timestamp(0, 0).unwrap().naive_utc(); | ||
| let duration = chrono::Duration::microseconds(microseconds); | ||
| let datetime = epoch + duration; |
There was a problem hiding this comment.
Could we use checked_add_signed here and return None when the result is outside Chrono’s date range? duration_from_precision_units can produce a valid Duration whose addition to the epoch overflows NaiveDateTime; the + implementation panics in that case. Textification should report an invalid/unrepresentable protobuf value rather than panic.
| fn precision_time_to_string(value: i64, precision: i32) -> Option<String> { | ||
| let duration = duration_from_precision_units(value, precision)?; | ||
| let midnight = NaiveTime::from_hms_opt(0, 0, 0).unwrap(); | ||
| let time = midnight + duration; |
There was a problem hiding this comment.
NaiveTime + Duration wraps modulo 24 hours, so an invalid precision-time value such as 86,460 seconds silently textifies as 00:01:00. Since this is a time since midnight rather than a duration, could we validate that the value is within one day and emit a failure token/diagnostic when it is not? That keeps malformed protobuf input visible instead of changing its value.
| - Examples: `'2023-01-01':date`, `'2023-12-25T14:30:45.123':timestamp`, `'2023-01-01T12:00:00.123456789':precisiontimestamp<9>`, `'14:30:45.123456':precisiontime<6>` | ||
|
|
||
| All basic literal types (`integer`, `float`, `boolean`, and `string`) are supported, plus `date`, `time`, `timestamp`, and typed null literals. Other Substrait literal types (e.g., `interval_year`, `decimal`, `uuid`) are not yet implemented. | ||
| All basic literal types (`integer`, `float`, `boolean`, and `string`) are supported, plus `date`, `time`, `timestamp`, `precisiontime`, `precisiontimestamp`, `precisiontimestamptz`, and typed null literals. Other Substrait literal types (e.g., `interval_year`, `decimal`, `uuid`) are not yet implemented. |
There was a problem hiding this comment.
Could we document that the deprecated timestamp_tz literal is still unsupported, and recommend precisiontimestamptz<6> instead? I am fine with leaving the deprecated form unimplemented, but this support summary currently makes that boundary easy to miss.
gord02
changed the title
gord02
changed the title
2 participants
Description
Adds text-format parsing and textification for Substrait's precisiontimestamp, precisiontimestamptz, and precisiontime literals (values carrying an explicit precision of 0/3/6/9/12 — seconds through picoseconds), which previously had no literal support.
Details
Parser (src/parser/expressions.rs): Precision 12 (picoseconds) is rejected at parse time — chrono can't represent it — with a clear error; overflow also errors instead of silently truncating.
Textify (src/textify/expressions.rs): Formatting is best-effort per the project's design: precision 12 is truncated to nanoseconds and rendered (rather than replaced with an error token), with a warning pushed noting the loss. The emitted type suffix reports the truncated precision (9), not the original 12, so output stays parseable by this crate's own parser.
Grammar (GRAMMAR.md): documents the new precisiontime/precisiontimestamp/precisiontimestamptz syntax and that only precisions 0/3/6/9 are supported.
Tests (src/types_tests.rs, inline unit tests): round-trip coverage for all supported precisions across all three variants, plus best-effort/error-path coverage for precision 12 and unsupported precisions.
No changes to the legacy (deprecated) time/timestamp literal types, which remain fixed at microsecond precision per the Substrait spec.
Type of Change
Testing
Related Issues
Closes #49