feat: interval day to second support by gord02 · Pull Request #183 · DataDog/substrait-explain

gord02 · 2026-07-14T17:13:21Z

Description

Adds Substrait's IntervalDayToSecond type and literal to the text format, on both the parse and textify sides.

Type: interval_day (bare, unset precision → treated as 6/microseconds) and interval_day (explicit precision 0–12), matching Type.IntervalDay in the spec.
Literal: '':interval_day, e.g. '5d 3s':interval_day, '123456789ns':interval_day. A literal is 1–3 whitespace-separated terms (d, s, and one of ms/us/ns/ps), each term optionally signed and appearing at most once; the subsecond unit determines the stored precision. Literal type ascription is always bare :interval_day — precision is derived from the string, not a type parameter — to avoid two ways of expressing precision disagreeing with each other.
Enforces the spec's ±3,650,000-day bound (both individually and combined with seconds), i32 bounds on seconds, and per-precision bounds on subseconds.
Updates GRAMMAR.md with the interval_day_literal grammar and precision semantics.

Type of Change

New feature

Testing

Added tests for new functionality
All existing tests pass

Related Issues

Closes #177

Copilot

Pull request overview

Adds IntervalDayToSecond support to the substrait-explain text format, covering both parsing (text → protobuf literal/type) and textification (protobuf → canonical text), and updates the documented grammar accordingly.

Changes:

Adds interval_day typed literal parsing/textification using a compact duration-string encoding (e.g. '4d 5s':interval_day, '123456789ns':interval_day) with bounds validation.
Adds interval_day type support, including bare interval_day (unset precision) and parameterized interval_day<precision> (0–12).
Extends grammar + tests for roundtrip coverage of the new type and literal behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/literal_roundtrip.rs	Adds a plan-level roundtrip test covering an `interval_day` literal.
src/types_tests.rs	Adds type/literal roundtrip assertions for `interval_day` and representative literals.
src/textify/types.rs	Emits bare `interval_day` when precision is unset; emits `interval_day<precision>` when set.
src/textify/expressions.rs	Implements IntervalDayToSecond literal textification as a duration string + adds validation tests.
src/parser/types.rs	Parses bare `interval_day` and parameterized `interval_day<precision>` with bounded precision validation.
src/parser/expressions.rs	Parses `:interval_day` typed string literals into IntervalDayToSecond with bounds/format checks.
src/parser/expression_grammar.pest	Adds `interval_day` to type grammar and introduces `interval_day_type`.
GRAMMAR.md	Documents `interval_day` typed-literal and type syntax/semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+interval_day_literal := duration_term (" " duration_term)*
+duration_term := "-"? digit+ ("d" / "s" / "ms" / "us" / "ns" / "ps")
+```


+            // An all-zero value at precision 0 collapses to the same "0s" as the
+            // all-zero default (precision 6): precision 0 has no subsecond term
+            // at all, so there's nothing to distinguish it from the default once
+            // days and seconds are also 0. Re-parsing "0s" always yields
+            // precision 6, so this specific value does not round-trip.


chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8809f5719f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-07-15T19:39:25Z

+    let (precision, subseconds) = match interval.precision_mode {
+        Some(PrecisionMode::Precision(p)) => (p, interval.subseconds),
+        Some(PrecisionMode::Microseconds(us)) => (6, us as i64),
+        None => (0, interval.subseconds),


Preserve unset interval precision as microseconds

When textifying protobufs where IntervalDayToSecond.precision_mode is unset, this maps the value to precision 0 and emits text like '5d':interval_day; parsing that text back creates Precision(0). The Substrait compatibility behavior for older/unset interval literals is microsecond precision, so inputs with precision_mode: None (common from older plans or default-valued protos) lose their precision on protobuf→text→protobuf round trips; this should emit a zero us component or otherwise preserve precision 6.

Useful? React with 👍 / 👎.

This is a good point but is potentially okay to leave as is. Zero makes the most sense as the default.

After looking into it further, I don't think this is a case worth covering. precision is required in the proto and the specs have no mention of being able to not set a precision.

wackywendell · 2026-07-29T20:21:44Z

  | "date"
  | "time"
  | "interval_year"
+  | "interval_day"


Why is this here? interval_day is not a 'simple type', its a compound type.

wackywendell · 2026-07-29T20:22:00Z

+// The trailing "<" lookahead is rejected so that "interval_day<6>" is left for
+// interval_day_type (below) instead of being swallowed as bare "interval_day".
+simple_type = { simple_type_name ~ nullability ~ !"<" }


Let's undo this as well; it shouldn't be here in simple_type.

wackywendell · 2026-07-29T20:22:31Z

+// The parameterized interval_day<precision> compound type. Bare "interval_day"
+// (no parameters) is handled by simple_type_name above instead.


What is this about a 'bare' "interval_day" type? Why are we supporting that?

I... don't think we should do that. For every other type with parameters that we support, we require the parameters to be written out. I think we should do that here, as well.

Looking at the issue - I see I suggested '30s':interval_day; but I think leaving off the <0> was accidental there, and I don't know that we want to set a precedent for that.

wackywendell · 2026-07-29T20:36:51Z


+/// Split a single duration term (e.g. "5d", "-3s", "123ns") into its signed numeric
+/// part and unit suffix.
+fn split_duration_term<'a>(


I would suggest using Pest here; make a rule for it in Pest, which will be easier to read and give better error messages when it fails to parse, and then parse the pieces of it.

// One day term, one subseconds term, or both. interval_day_duration = { SOI ~ ( duration_day ~ (" " ~ duration_seconds)? ~ (" " ~ duration_subseconds)? | duration_seconds ~ (" " ~ duration_subseconds)? | duration_subseconds ) ~ EOI } duration_day = { integer ~ "d" } duration_seconds = { integer ~ "s" } duration_subseconds = { integer ~ ("ms" | "us" | "ns") }

Note also:

This allows d, s, and ms/us/ns, in that order. Each is optional, at least one required.

It only allows a single space between, and no leading/trailing, and no tabs. That's a bit strict, but... seems reasonable? Do you really need any of this?

Then... this can go in the main grammar file, but I would suggest not including it as a sub-rule anywhere; I think that will get complicated - I'd just have the Pest parser parse this again, once you got to this level in the code, ExpressionParser::parse(Rule::interval_day_duration, unescaped_string).

wackywendell · 2026-07-29T20:37:16Z

+/// Mark a duration term category (e.g. "'d'", "subsecond (ns)") as seen, erroring
+/// if it was already seen. Each unit category (`d`, `s`, one subsecond unit) may
+/// appear at most once in a duration string.
+fn mark_seen(


See above - with a Pest grammar like the above, the grammar would cover that.

wackywendell · 2026-07-29T21:02:44Z

+    };
+
+    const MAX_DAYS: i32 = 3_650_000;
+    if !(-MAX_DAYS..=MAX_DAYS).contains(&interval.days) {


While this is technically invalid substrait, I think you shouldn't refuse to write it - just write it, but also put an error into the error channel (via ctx.push_error).

wackywendell · 2026-07-29T21:04:29Z

+        return write!(
+            w,
+            "{}",
+            ctx.failure(PlanError::invalid(


As above - push a warning, no need to ctx.failure here. The printer is supposed to be best-effort outputting, not strict - it should output anything it finds as long as it has a way to; failure tokens are only for cases where our syntax/grammar doesn't have a reasonable way to output it.

wackywendell · 2026-07-29T21:06:44Z

+```text
+interval_day_literal := duration_term (whitespace duration_term)? (whitespace duration_term)?
+duration_term        := "-"? digit+ duration_unit
+duration_unit        := "d" / "s" / "ms" / "us" / "ns" / "ps"


As above - I think it makes sense to:

only allow a single space, not multiple, not tabs. We accept more in the broader grammar, but in literals, no need to.

Only allow descending order, and only one subsecond unit.

You can encode that into the PEG grammar here, too.

wackywendell · 2026-07-29T21:07:00Z

- `interval_year`, `uuid`
+- `interval_year`, `interval_day`, `uuid`
+
+Bare `interval_day` is accepted as a type name, with an unset precision


I don't think we should support this. precision is a required field (now and here), let's require the user to be explicit.

wackywendell · 2026-07-29T21:23:05Z

+        ));
+    }
+
+    // Per the Substrait spec, IntervalDayToSecond supports a range of [-3,650,000..3,650,000] days.


Generally, we don't want to be stricter than necessary on Substrait - this isn't a validator, its a converter.

Generally - if 'invalid' Substrait has a clear syntax (as out-of-range seconds does), there's no need to go out of our way to reject it. Ideally, we would output a warning (as textify can), but in the parser, I'd suggest a // TODO note here about validation and just accept it.

feat: adding support for compound interval day type feat: cleaning up code fix: updating default value feat: cleaning up code feat: forcing precision suffix

Reuse the precision type from #178 instead of introducing a second one, and repair two function renames the rebase left behind. - Moved `SupportedPrecision` from `parser/expressions.rs` to a shared `precision` module, added a `Picoseconds` variant, and gave it the duration-unit mapping `interval_day` needs. The chrono-backed literals still reject precision 12 in `check_supported_precision`; `interval_day` accepts it, since sub-seconds are a plain integer count there. - `interval_day` literals now require a precision with a unit (0, 3, 6, 9, or 12), matching how precisiontimestamp/precisiontime literals behave. The type itself still accepts any precision from 0 to 12. - Type-level `parse_precision` stays a plain 0..=12 integer; narrowing to a writable precision is a literal concern. - `write_literal_type_suffix` gained an `interval_day` arm, replacing the bespoke suffix handling. - Restored `precision_timestamp_to_string` and `precision_time_to_string`, which the rebase had renamed and cross-wired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Addresses review feedback on #183: an outer function owns the re-parse and the span/error handling, and a second function converts the resulting `Pair` into an `IntervalDayToSecond`. - `parse_interval_day_duration` unescapes-string input, runs `ExpressionParser::parse(Rule::interval_day_duration, ..)`, and turns either step's failure into a `MessageParseError` with the literal's span. - `interval_day_from_pair` takes the matched pair plus the ascribed precision and returns a new `IntervalDayError` for what it can find - a term too large for its protobuf field, or a sub-second unit that disagrees with the precision. No span is threaded through: the re-parsed string's spans point into a temporary, so the caller supplies the span. - `use pest::Parser as PestParser` moved to the top of the file, replacing the fully-qualified `<ExpressionParser as pest::Parser<Rule>>::parse`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

wackywendell

This is looking much better! The split between parse_interval_day_duration and interval_day_from_pair makes sense.

I have a couple code organization comments, but otherwise, LGTM!

wackywendell · 2026-08-06T21:09:38Z

 // Literal
 literal = { (float | integer | boolean | string_literal | null) ~ (":" ~ sp ~ type)? }

+// -- interval_day durations --


This section should probably go at the end of the file, or even in its own separate file, since its not really part of the same "grammar" in the sense that its parsed separately...

wackywendell · 2026-08-06T21:18:08Z

+                "interval_day_duration",
+                span,
+                format!(
+                    "Invalid duration '{duration_str}': {}. Expected one to three terms, separated by single spaces, in the order days ('d'), seconds ('s'), sub-seconds ('ms', 'us', 'ns', or 'ps'); e.g. '5d', '4d 5s', '5d 3s 100ns'",


Good error message 👍

wackywendell · 2026-08-06T21:19:55Z

+/// values recorded before `precision_mode` existed, and it matches the default
+/// the type textifier uses for `Type.IntervalDay` with no precision.
+fn interval_day_precision_units(
+    interval: &substrait::proto::expression::literal::IntervalDayToSecond,


This should be use statement at the top, not a long inline like this

Suggested change

interval: &substrait::proto::expression::literal::IntervalDayToSecond,

interval: &IntervalDayToSecond,

wackywendell · 2026-08-06T21:22:44Z

@@ -0,0 +1,105 @@
+//! Sub-second precision for literal values, shared by the parser and textifier.


👍 , good module. 🤔 Organizationally, it might make sense to find a home for this in a directory in src rather than top-level like this, but its fine for now - and good that its all in one file 👍

gord02 requested a review from Copilot July 14, 2026 17:13

Copilot started reviewing on behalf of gord02 July 14, 2026 17:13 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

gord02 force-pushed the gordon.hamilton/interval-day-to-second-support branch from 462b480 to b39deaf Compare July 14, 2026 18:41

gord02 marked this pull request as ready for review July 15, 2026 19:33

gord02 requested review from a team and wackywendell as code owners July 15, 2026 19:33

chatgpt-codex-connector Bot reviewed Jul 15, 2026

View reviewed changes

wackywendell requested changes Jul 29, 2026

View reviewed changes

gord02 and others added 2 commits August 5, 2026 14:02

feat: implementing intveral day

9c5a336

feat: adding support for compound interval day type feat: cleaning up code fix: updating default value feat: cleaning up code feat: forcing precision suffix

gord02 force-pushed the gordon.hamilton/interval-day-to-second-support branch from e0fb04b to ca574fb Compare August 6, 2026 17:01

gord02 force-pushed the gordon.hamilton/interval-day-to-second-support branch from ca574fb to d214ffe Compare August 6, 2026 17:03

wackywendell approved these changes Aug 12, 2026

View reviewed changes

feat: addressing final review comments

8c0667d

gord02 force-pushed the gordon.hamilton/interval-day-to-second-support branch from 90171bb to 8c0667d Compare August 13, 2026 20:24

Merge branch 'main' into gordon.hamilton/interval-day-to-second-support

2700bcd

gord02 merged commit bd5aa43 into main Aug 14, 2026
4 checks passed

gord02 deleted the gordon.hamilton/interval-day-to-second-support branch August 14, 2026 15:02

wackywendell mentioned this pull request Aug 17, 2026

chore: release v0.9.0 #211

Merged

		// The parameterized interval_day<precision> compound type. Bare "interval_day"
		// (no parameters) is handled by simple_type_name above instead.

	interval: &substrait::proto::expression::literal::IntervalDayToSecond,
	interval: &IntervalDayToSecond,

		@@ -0,0 +1,105 @@
		//! Sub-second precision for literal values, shared by the parser and textifier.

Uh oh!

Conversation

gord02 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Testing

Related Issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

gord02 Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wackywendell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gord02 commented Jul 14, 2026 •

edited

Loading

gord02 Jul 22, 2026 •

edited

Loading