Skip to content

feat(spec): read timestamp_ns/timestamptz_ns default values in the Literal JSON codec#2832

Merged
blackmwk merged 2 commits into
apache:mainfrom
c-thiel:ct/timestamp-ns-default-values-json
Jul 16, 2026
Merged

feat(spec): read timestamp_ns/timestamptz_ns default values in the Literal JSON codec#2832
blackmwk merged 2 commits into
apache:mainfrom
c-thiel:ct/timestamp-ns-default-values-json

Conversation

@c-thiel

@c-thiel c-thiel commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Which issue does this PR close?

We don't have an issue for this. Deserializing Timestamp NS default values failed.

What changes are included in this PR?

try_into_json serialized TimestampNs/TimestamptzNs but try_from_json had no matching arms, so a nanosecond-timestamp single value (e.g. a column default) failed to deserialize (fell to the DataInvalid catch-all). Add the read arms
mirroring the microsecond ones, plus datetime_to_nanoseconds / datetimetz_to_nanoseconds helpers. Also adds json_timestamp_ns / json_timestamptz_ns tests.

Are these changes tested?

Yes

@Kurtiscwright Kurtiscwright left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks great.

QQ: Should we add tests with different time zones?

Mirror Java's testInvalidTimestamptz{,Nano}: only +00:00 is valid per
spec, so a non-UTC offset must be rejected.
@c-thiel

c-thiel commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Thank you, this looks great.

QQ: Should we add tests with different time zones?

Good call, added tests for it. Spec says the offset must be +00:00 and Java enforces the same (isUTCTimestamptz), so I added json_timestamptz_ns_rejects_non_utc_offset + the micros counterpart asserting a non-UTC offset is rejected (matching Java's testInvalidTimestamptz{,Nano}). +00:00 round-trip is already covered.

One note: we accept only the literal +00:00, not Z like Java does — spec mandates +00:00 and both writers emit it, so round-trips are fine. I would keep it strict for now.

@blackmwk blackmwk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @c-thiel for this fix!

@blackmwk blackmwk merged commit 743d098 into apache:main Jul 16, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants