Repository navigation
Fix timestamptz interval arithmetic - #4116
jeremy-morren wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
A critical UTC arithmetic bug remains for initially unmapped receivers with constant intervals, and the parity tests can pass despite client-side projection fallback.
2 open issues
What changed in this PR
This PR addresses timestamp and interval mismatches between PostgreSQL and .NET/NodaTime, covering session time zones, negative interval literals, and interval components.
Changes:
- Performs
timestamptzinterval arithmetic in UTC and simplifies redundant UTC conversions. - Corrects negative interval formatting and adds a compatibility switch for reading older JSON values.
- Shares interval-member translation and adds parity tests, mapping checks, and updated SQL baselines.
| File | Description |
|---|---|
| test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs | Tests signed interval literals and JSON round trips. |
| test/EFCore.PG.Tests/Storage/NpgsqlNodaTimeTypeMappingTest.cs | Updates negative Duration JSON expectations. |
| test/EFCore.PG.Tests/Storage/NpgsqlIntervalJsonParsingTest.cs | Tests both compatibility-switch settings and malformed JSON intervals. |
| test/EFCore.PG.FunctionalTests/Query/Translations/TimestampTranslationsTest.cs | Covers arithmetic, mapping inference, precision, and aliases. |
| test/EFCore.PG.FunctionalTests/Query/Translations/Temporal/TimeSpanTranslationsNpgsqlTest.cs | Updates interval-component SQL baselines. |
| test/EFCore.PG.FunctionalTests/Query/Translations/Temporal/DateTimeTranslationsNpgsqlTest.cs | Updates UTC arithmetic SQL baselines. |
| test/EFCore.PG.FunctionalTests/Query/Translations/Temporal/DateTimeOffsetTranslationsNpgsqlTest.cs | Updates UTC arithmetic SQL baselines. |
| test/EFCore.PG.FunctionalTests/Query/Translations/NodaTime/ZonedDateTimeTranslationsTest.cs | Updates Duration arithmetic SQL expectations. |
| test/EFCore.PG.FunctionalTests/Query/Translations/NodaTime/NodaTimeParityTest.cs | Adds database-versus-.NET arithmetic and component comparisons. |
| test/EFCore.PG.FunctionalTests/Query/Translations/NodaTime/NodaTimeParityFixture.cs | Checks expression type mappings before SQL generation. |
| test/EFCore.PG.FunctionalTests/Query/Translations/NodaTime/InstantTranslationsTest.cs | Tests arithmetic with timestamp precision and aliases. |
| test/EFCore.PG.FunctionalTests/Query/Translations/NodaTime/DurationTranslationsTest.cs | Adds milliseconds coverage and updates component baselines. |
| test/EFCore.PG.FunctionalTests/Query/OperatorsQueryNpgsqlTest.cs | Updates combined arithmetic and time-zone SQL. |
| test/EFCore.PG.FunctionalTests/Query/NorthwindMiscellaneousQueryNpgsqlTest.cs | Updates elapsed-day component SQL. |
| src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs | Initializes the legacy JSON interval parsing switch. |
| src/EFCore.PG/Storage/Internal/Mapping/NpgsqlIntervalTypeMapping.cs | Corrects interval signs and compatibility-aware parsing. |
| src/EFCore.PG/Query/NpgsqlSqlExpressionFactory.cs | Rewrites mapped timestamp arithmetic and recognizes timestamp aliases. |
| src/EFCore.PG/Query/Internal/NpgsqlQuerySqlGenerator.cs | Elides paired UTC conversions. |
| src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlTimeSpanMemberTranslator.cs | Shares normalized, truncating interval-member translation. |
| src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlDateTimeMethodTranslator.cs | Adds UTC handling for mapped timestamp addition methods. |
| src/EFCore.PG.NodaTime/Query/Internal/NpgsqlNodaTimeMemberTranslatorPlugin.cs | Reuses shared translation for Duration members. |
Review effort: Balanced
a8b09eb to
b659b4f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues cause unmapped DateOnly parameter queries to fail and produce incorrect legacy timestamp calendar arithmetic across daylight-saving transitions.
2 open issues
2 resolved since last review
Review effort: Balanced
… and components Date/time arithmetic on timestamptz followed the session time zone (npgsql#4087): PostgreSQL's timestamptz +/- interval adds the interval's days and months as calendar units in the session TimeZone (https://www.postgresql.org/docs/current/functions-datetime.html, timestamptz_pl_interval_internal: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/utils/adt/timestamp.c#L3254-L3381), so e.g. adding a day across a daylight saving time transition adds 23 or 25 hours. NodaTime's Instant/ZonedDateTime +/- Duration is exact, and DateTime/DateTimeOffset values mapped to timestamptz are UTC; this affected both the +/- operators (Duration/TimeSpan) and DateTime/DateTimeOffset.AddYears/Months/Days/... The arithmetic is now performed on UTC timestamps: (x AT TIME ZONE 'UTC' + interval) AT TIME ZONE 'UTC'. A resulting (y AT TIME ZONE 'UTC') AT TIME ZONE 'UTC' pair is elided in SQL generation. Negative TimeSpan/Duration literals with a days component were wrong (npgsql#4086): PostgreSQL applies a sign to the field it precedes only (https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT), so '-1 02:00:00' is -22 hours, not -26. Negative values are now generated with a sign on both the days and the time ('-1 -02:00:00'); the JSON readers still accept the old format. TimeSpan/Duration components were wrong for negative values and for intervals computed in SQL (npgsql#4088): .Days/.Hours/.Minutes/.Seconds/.Milliseconds read the raw interval fields with floor(), but .NET and NodaTime truncate towards zero, and an interval computed in SQL keeps separate (possibly differently-signed) day and time fields, with a time field which can exceed a day. The components are now read from justify_hours(x), which moves whole days into the day field and leaves both fields with the same sign (interval_justify_hours: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/utils/adt/timestamp.c#L3022-L3056), and truncated. Adds NodaTimeParityTest, which evaluates each case (a lambda plus its arguments) in PostgreSQL, with the arguments both as parameters and as constants, and via Compile(), asserting identical results; its fixture uses a session time zone with daylight saving time, and verifies that every SQL expression involving a NodaTime type has a matching type mapping. Adding a case is a single line. Fixes npgsql#4086 Fixes npgsql#4087 Fixes npgsql#4088 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vn7k8XUqiuZ5UX1CKpizxU
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b659b4f to
e74be9a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Interval parsing rejects valid values such as 10675200 -23:00:00 because it range-checks the day component before combining it with the opposing time component.
1 open issue
2 resolved since last review
Review effort: Balanced
…liseconds, share TimeSpan/Duration members
* Reading a negative interval with a days component and a sign on the days only (e.g.
'-1 02:00:00') from JSON is ambiguous: older versions of the provider wrote negative values
that way, meaning that the whole value is negative (-26 hours), whereas for PostgreSQL the
sign applies to the days only (-22 hours). Such values keep the provider's old meaning by
default; the new Npgsql.ParseOldEFCoreJsonTimeSpans AppContext switch can be turned off to give
them PostgreSQL's meaning instead, e.g. when the JSON was written by another application.
Values written by the current version ('-1 -02:00:00') are unambiguous.
* Translate NodaTime Duration.Milliseconds, which is truncated towards zero like
TimeSpan.Milliseconds
(https://github.com/nodatime/nodatime/blob/3.2.2/src/NodaTime/Duration.cs#L258).
* TimeSpan and Duration have the same component and total members, with the same semantics;
they're now translated by a single shared method
(NpgsqlTimeSpanMemberTranslator.TranslateIntervalMember).
* Test that arithmetic on timestamp (without time zone) isn't performed in UTC.
* NodaTimeParityTest generates the exact same arithmetic and member cases for TimeSpan and
Duration. TotalDays...TotalMilliseconds are compared within a small tolerance, as the
existing date_part('epoch', x) / n translation isn't bit-exact (to be handled separately).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Covers ParseIntervalAsTimeSpan's error paths (no time component, a days component in the time, more than one sign on the time), which dotCover showed as uncovered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ime and PostgreSQL Each case states its correct result, which .NET/NodaTime itself must return (checking the case), as must PostgreSQL. The TimeSpan/Duration cases cover intervals computed in SQL whose fields have the same or opposite signs, or a time field of a day or more, and Interval_components_require_justify_hours checks that justify_hours() is required exactly for the latter two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e74be9a to
a30b0f1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unconditional interval normalization breaks valid TimeOfDay component queries, while UTC-based month/year arithmetic produces incorrect results for legacy DateTimeOffset values with nonzero offsets.
2 open issues
1 resolved since last review
Review effort: Balanced
| SqlExpression JustifyHours() | ||
| => sqlExpressionFactory.Function( | ||
| "justify_hours", | ||
| [interval], | ||
| nullable: true, | ||
| argumentsPropagateNullability: TrueArrays[1], | ||
| interval.Type, | ||
| interval.TypeMapping); |
| // in the session time zone; so it's left as-is. | ||
| if (IsTimestampTz(resultMapping) | ||
| && (right.Type == typeof(TimeSpan) || right.Type.FullName == "NodaTime.Duration") | ||
| && !(NpgsqlTypeMappingSource.LegacyTimestampBehavior && binary.Type == typeof(DateTime))) |


Fixes three bugs in date/time arithmetic with intervals, all of which make PostgreSQL disagree with the .NET/NodaTime result.
Arithmetic on
timestamptzfollowed the session time zone (#4087). PostgreSQL'stimestamptz +- intervaladds the interval's days and months as calendar units in the sessionTimeZone(seetimestamptz_pl_interval_internal). (docs: "adding or subtracting (say)interval '1 day'across DST is 23 or 25 hours. This affectedInstant/ZonedDateTime +- Duration,DateTime/DateTimeOffset +- TimeSpan, andDateTime/DateTimeOffset.AddYears/Months/Days/.... The arithmetic is now performed on UTC timestamps,(x AT TIME ZONE 'UTC' + interval) AT TIME ZONE 'UTC'(which also handles days fields from stored data and fromtimestamptz - timestamptz). A resulting(y AT TIME ZONE 'UTC') AT TIME ZONE 'UTC'pair is elided during SQL generation. The rewrite is applied once the timestamp's type mapping is known, so that an unmapped operand (e.g. aDateTimeparameter) can still be inferred astimestamp without time zonefrom the other side of a comparison, andtimestamptzis recognized by itsNpgsqlDbType, so that aliases (timestamptz) and precisions (timestamp(3) with time zone) are included;AT TIME ZONE's result type is now also inferred that way.Negative
TimeSpan/Durationliterals with a days component were wrong (#4086). PostgreSQL applies a sign to the field it precedes only (docs), soINTERVAL '-1 02:00:00'is −22 hours rather than −26. Negative values are now generated with a sign on both fields ('-1 -02:00:00'), in SQL literals and in JSON.Reading a negative value with a days component and a sign on the days only (e.g.
"-1 02:00:00") from JSON is therefore ambiguous: older versions of the provider wrote negative values that way, meaning that the whole value is negative (−26 hours), whereas for PostgreSQL the sign applies to the days only (−22 hours). This is controlled by a new AppContext switch, in line with the existingNpgsql.EnableLegacyTimestampBehavior/Npgsql.DisableDateTimeInfinityConversionsones:Npgsql.ParseOldEFCoreJsonTimeSpans(on by default): such values are given the provider's old meaning, so JSON written by previous versions keeps reading back unchanged.Turn it off when the JSON was written by another application which intended PostgreSQL's meaning:
Values written by this version are unambiguous, and read the same either way; values without a days component, or without a negative sign, aren't affected. This applies to both
TimeSpanand NodaTimeDuration.The switch only applies to JSON read in .NET: a query which evaluates a JSON member in PostgreSQL converts it to
intervalwith PostgreSQL's meaning, which can't be special-cased. Existing JSON data with negative values with days therefore needs to be converted manually; the release notes document this (with SQL to do so), and that previous versions of the provider misread the new format (relevant for rolling deployments and rollbacks).TimeSpan/Durationcomponents were wrong for negative values and for intervals computed in SQL (#4088)..Days/.Hours/.Minutes/.Seconds/.Millisecondsread the raw interval fields withfloor(), whereas .NET and NodaTime truncate towards zero; and an interval computed in SQL keeps separate, possibly differently-signed day and time fields (e.g.1 day -01:00:00), with a time field that can exceed a day. The components are now read fromjustify_hours(x), which moves whole days into the day field and leaves the two fields with the same sign (seeinterval_justify_hours), and truncated.Duration.Milliseconds, which wasn't translated, is now translated too (truncated towards zero, like NodaTime andTimeSpan.Milliseconds). SinceTimeSpanandDurationhave the same component and total members with the same semantics, both are now translated by a single shared method.Tests
Adds
NodaTimeParityTest: each case is a single line — a lambda, its arguments and its expected (correct) result. The lambda is evaluated in .NET (viaCompile()), which must return the expected result (so each case's expectation is checked against .NET/NodaTime itself), and in PostgreSQL, with the arguments sent both as parameters (EF.Parameter) and inlined as constants (EF.Constant, covering literal generation), which must also return the expected result; the test verifies that the query was actually evaluated in the database. Known discrepancies are skipped, but a separate test still checks their expected result against .NET/NodaTime. The fixture's session time zone (Europe/Berlin) has daylight saving time, and it verifies that every SQL expression involving a NodaTime type has a type mapping for that type, and thatdate_part()/trunc()are typed asdouble precision. It also has a section for the BCLDateTime/DateTimeOffsetcases above, and generates the exact same arithmetic and member cases forTimeSpanandDuration, from a table of values with their expected components.The
TimeSpan/Durationcases include intervals computed in SQL whose day and time fields have the same sign, opposite signs (e.g.1 day -01:00:00), or a time field of a day or more (e.g.40:00:00).Interval_components_require_justify_hoursreads the components of each of these with and withoutjustify_hours(), and checks thatjustify_hours()is required exactly for the latter two.TotalDays...TotalMillisecondsare compared within a small floating-point tolerance: the existingdate_part('epoch', x) / ntranslation (unchanged here) isn't bit-exact, as it rounds twice, unlikeTimeSpan'sticks / n. Making them bit-exact is left for a separate issue.Also adds tests for reading JSON intervals with both values of
Npgsql.ParseOldEFCoreJsonTimeSpans, for arithmetic ontimestamp(without time zone), which isn't affected by the time zone change, for arithmetic on aDateTimeparameter whose mapping is inferred from atimestamportimestamptzcolumn on either side of a comparison, and for arithmetic ontimestamptzcolumns with a precision or thetimestamptzalias (DateTime,DateTimeOffset,InstantandZonedDateTime). Existing SQL baselines are updated.Fixes #4086
Fixes #4087
Fixes #4088