feat!: remove deprecated time, timestamp and timestamp_tz types by nielspardon · Pull Request #994 · substrait-io/substrait

nielspardon · 2026-03-05T14:59:22Z

BREAKING CHANGE: removes the deprecated time, timestamp and timestamp_tz types from:

proto files
dialect schema
extension yamls
ANTLR grammar
test cases
coverage python code
documentation

closes #980

This change is

nielspardon · 2026-03-05T15:27:58Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return: |-
+          precision = integer_parameter(precision)
+          precision_timestamp<precision>


introducing a new precision argument here, to make the return type precision predictable

I have no idea what the level of support is for interger_parameter actually is in the downstream libraries. It doesn't seem very well specified, considering that the only line about it is:

integer_parameter(string name) => integer

I propose we tackle this in a separate PR where we can have dedicated discussion about it.

btw I had already created an issue for improving the documentation of these return type functions and had asked @jacques-n if he wants to contribute since he originally introduced these functions (unfortunately did not get any reaction from him) #959

nielspardon · 2026-03-05T15:28:13Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return:  |-
+          precision = integer_parameter(precision)
+          precision_timestamp_tz<precision>


introducing a new precision argument here, to make the return type precision predictable

nielspardon · 2026-03-05T15:28:19Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return: |-
+          precision = integer_parameter(precision)
+          precision_time<precision>


introducing a new precision argument here, to make the return type precision predictable

nielspardon · 2026-03-05T15:28:25Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return: |-
+          precision = integer_parameter(precision)
+          precision_timestamp_tz<precision>


introducing a new precision argument here, to make the return type precision predictable

nielspardon · 2026-03-05T15:28:32Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return: |-
+          precision = integer_parameter(precision)
+          precision_timestamp_tz<precision>


introducing a new precision argument here, to make the return type precision predictable

nielspardon · 2026-03-05T16:05:48Z

tools/proto_prefix.py

        string=re.compile(r'"(?:[^"\\]|\\.)*"'),
        number=re.compile(r"[0-9]+"),
-        symbol=re.compile(r"[=;{}\[\]]"),
+        symbol=re.compile(r"[=;,{}\[\]]"),


since we now have the reserved 14, 17, 27; statements in the proto files we need to add comma , to the symbol regex

yongchul

Overall, okay with the change but I want us to discuss and document deprecation and release policy... for instance, @vbarua proposed to hold off a breaking change and make an explicit release with the breaking change (#971 (comment)) -- which to me seems a good idea.

Will create a discussion item for this.

benbellick · 2026-03-10T13:56:47Z

I haven't investigated myself yet but has this migration been managed within the reasonably maintained implementation libraries? (For us, this means substrait-{go, java, py, rs})

nielspardon · 2026-03-10T18:08:33Z

I haven't investigated myself yet but has this migration been managed within the reasonably maintained implementation libraries? (For us, this means substrait-{go, java, py, rs})

Here the problem is that there is no clean migration path right now since some datetime function variants simply do not exist for the new types in the current default extensions. We would need to at least introduce the new function variants.

Even then I would argue the time and timestamp types have been deprecated for a long time and it should be ok to force a migration to the new types by removing the old types.

yongchul · 2026-03-20T18:33:04Z

My previous comment still holds so I'm okay to do this breaking change. But since we have deprecation mechanism, that is another option and delay this removal a few releases (i.e., make this PR as official deprecation then remove).

nielspardon · 2026-03-20T19:14:58Z

My previous comment still holds so I'm okay to do this breaking change. But since we have deprecation mechanism, that is another option and delay this removal a few releases (i.e., make this PR as official deprecation then remove).

I guess you mean what you introduced in #1014 with deprecation mechanism which is for extensions. This PR mainly removes the data types from the protobuf files which have been deprecated for a while now and as consequence of that removes / updates functions using the data types. The protobuf deprecation already led to deprecation warnings in the code generated from the protobufs (at least in Java) ever since they were marked deprecated. Marking the extensions deprecated does not lead to any warnings right now and is mainly informational. Marking the extensions using long deprecated types as deprecated first would satisfy the new process but feels a bit redundant for such a longstanding deprecation especially since there are no warning mechanisms in place anyway.

benbellick · 2026-03-20T19:19:03Z

Yeah I'm +1 with @nielspardon about just doing this breaking change. I just need to validate that Datafusion consumer actually accepts the new value. I will do that right now!

benbellick · 2026-03-20T19:34:49Z

Seems like we are good on the DF side of things:

Type Handling

PrecisionTimestamp - converts to Arrow Timestamp with precision-derived TimeUnit
PrecisionTimestampTz - converts to Arrow Timestamp with UTC timezone
PrecisionTime - converts to Arrow Time32/Time64 based on precision

Literal Handling

PrecisionTimestamp literals - consumes literal values with precision 0/3/6/9 → Second/Milli/Micro/Nano
PrecisionTimestampTz literals - consumes timezone-aware literal values
PrecisionTime literals - consumes time literal values as Time32/Time64 scalars

benbellick

Mostly looks good to me. Only a few minor things I flagged in comments. Thanks!

benbellick · 2026-03-20T19:37:09Z

tests/baseline.json

    "num_aggregate_functions": 29,
    "num_scalar_functions": 170,
    "num_window_functions": 11,
-    "num_function_overloads": 533


Why did this one go up so much?

I've been asking Claude for an explanation and it gave a very detailed one:

When the deprecated time, timestamp, and timestamp_tz types were removed, they were replaced with parameterized types like precision_time
and precision_timestamp
.

The massive increase in overloads (533 → 4,482) is due to parametric type expansion. Previously, functions like extract(HOUR, x: timestamp) had a single overload. Now with precision_timestamp
, the type system generates separate overloads for each supported precision value (microsecond, millisecond, etc.), causing a combinatorial explosion in the number of overload signatures.

For example:

Old: 1 overload for extract(HOUR, x: timestamp)
New: Multiple overloads for extract(HOUR, x: precision_timestamp), extract(HOUR, x: precision_timestamp), etc.
This pattern repeated across many datetime functions explains the ~8.4× increase. The later jump from 4,482 → 8,466 in commit 63f3fd5 likely comes from additional function additions or test coverage adjustments.

Hmm... if that is the case, what do you think about only deleting the old impls in this PR, and we can add new ones if we need in a subsequent one?

I'm only adding new impls when there would be no equivalent impls after removing the old ones. This PR retain parity in terms of functionality with before the change aka if there was an impl with time, timestamp, timestamp_tz before and there wouldn't be one after removing all impls using those then I'm adding the equivalent precision impl. If we do these in separate PRs then we risk having a Substrait release that has less functionality than before.

I have no idea what's going on here. The explanation that Claude Code gave does not match the current code on main. Currently, num_function_overloads is essentially the total number of impls across all extension YAMLs. I rebased this PR branch on latest main and now num_function_overloads decreases now from 533 to 498 which sounds more reasonable than the previous 8,466.

tests/cases/datetime/add_datetime.test

benbellick · 2026-03-20T19:40:50Z

extensions/functions_datetime.yaml

+          - name: precision
+            value: i8
+        return: |-
+          precision = integer_parameter(precision)
+          precision_timestamp<precision>


I have no idea what the level of support is for interger_parameter actually is in the downstream libraries. It doesn't seem very well specified, considering that the only line about it is:

integer_parameter(string name) => integer

I propose we tackle this in a separate PR where we can have dedicated discussion about it.

BREAKING CHANGE Signed-off-by: Niels Pardon <par@zurich.ibm.com>

Signed-off-by: Niels Pardon <par@zurich.ibm.com>

nielspardon force-pushed the par-rm-depr-datetime branch 4 times, most recently from 250d6ab to de5bf21 Compare March 5, 2026 15:27

nielspardon commented Mar 5, 2026

View reviewed changes

nielspardon force-pushed the par-rm-depr-datetime branch 2 times, most recently from bc6f5bc to a371761 Compare March 5, 2026 16:04

nielspardon commented Mar 5, 2026

View reviewed changes

nielspardon marked this pull request as ready for review March 5, 2026 16:06

nielspardon requested review from EpsilonPrime, cpcloud, jacques-n, vbarua, westonpace and yongchul as code owners March 5, 2026 16:06

This was referenced Mar 5, 2026

drop support for deprecated time types #980

Open

feat(isthmus): migrate to PrecisionTime / PrecisionTimestamp substrait-io/substrait-java#721

Merged

yongchul approved these changes Mar 5, 2026

View reviewed changes

nielspardon force-pushed the par-rm-depr-datetime branch from a371761 to 4dc6368 Compare March 9, 2026 15:32

benbellick reviewed Mar 20, 2026

View reviewed changes

nielspardon force-pushed the par-rm-depr-datetime branch from 4dc6368 to 0eecb71 Compare March 20, 2026 19:48

nielspardon added 3 commits March 23, 2026 13:36

feat!: remove deprecated time, timestamp and timestamp_tz types

84dcdbd

BREAKING CHANGE Signed-off-by: Niels Pardon <par@zurich.ibm.com>

fix: tests and update baseline

b581270

Signed-off-by: Niels Pardon <par@zurich.ibm.com>

fix: test coverage baseline

220f284

Signed-off-by: Niels Pardon <par@zurich.ibm.com>

nielspardon force-pushed the par-rm-depr-datetime branch from 63f3fd5 to 220f284 Compare March 23, 2026 12:43

nielspardon added the PMC Ready PRs ready for review by PMCs label Mar 23, 2026

nielspardon self-assigned this Mar 23, 2026

Conversation

nielspardon commented Mar 5, 2026 • edited by jacques-n Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

yongchul left a comment • 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!

benbellick commented Mar 10, 2026

Uh oh!

nielspardon commented Mar 10, 2026

Uh oh!

yongchul commented Mar 20, 2026

Uh oh!

nielspardon commented Mar 20, 2026

Uh oh!

benbellick commented Mar 20, 2026

Uh oh!

benbellick commented Mar 20, 2026

Type Handling

Literal Handling

Uh oh!

benbellick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielspardon Mar 20, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nielspardon commented Mar 5, 2026 •

edited by jacques-n

Loading

yongchul left a comment •

edited

Loading

nielspardon Mar 20, 2026 •

edited

Loading