Conversation
In PostgreSQL's binary wire protocol, timestamps are encoded as microseconds relative to 2000-01-01 (the PostgreSQL epoch). Previously, TimestampParser.toTimestamp(byte[]) passed the raw microsecond value directly to Timestamp.ofTimeMicroseconds(pgMicros) before adding the 30-year epoch offset (PG_EPOCH_SECONDS). Because Timestamp.ofTimeMicroseconds checks bounds against the 1970 Unix epoch, any date between 0001-01-01 and ~0031-01-01 was treated as falling before the minimum supported timestamp, throwing an uncaught IllegalArgumentException (SQLState P0001). This change: - Decomposes PostgreSQL microseconds into whole seconds and remaining microseconds using Math.floorDiv and Math.floorMod, correctly adjusting seconds for negative offsets before year 2000. - Adds PG_EPOCH_SECONDS to obtain the Unix epoch seconds before constructing the Timestamp. - Catches IllegalArgumentException for timestamps genuinely outside Spanner's supported range (before 0001-01-01 or after 9999-12-31) and returns SQLState.DatetimeFieldOverflow (22008) with message "timestamp out of range", matching PostgreSQL behavior. - Adds unit tests for dates across years 0001–0030, boundary limits, and out-of-range inputs.
There was a problem hiding this comment.
Code Review
This pull request improves timestamp parsing in TimestampParser to correctly handle negative microsecond offsets (timestamps before 2000-01-01) using floor division and modulo, and adds robust range validation that throws a PostgreSQL-compatible datetime field overflow exception. Comprehensive unit tests have been added to verify edge cases, including earliest/latest supported timestamps and out-of-range values. The reviewer suggested optimizing the remainder calculation in TimestampParser by replacing Math.floorMod with a faster multiplication and subtraction operation using the already computed seconds.
| // Use floor division and floor modulo so that negative microsecond offsets (timestamps before | ||
| // 2000-01-01) properly adjust whole seconds and produce a non-negative fractional remainder. | ||
| long pgSeconds = Math.floorDiv(pgMicroseconds, MICROSECONDS_IN_SECOND); | ||
| long remainingMicroseconds = Math.floorMod(pgMicroseconds, MICROSECONDS_IN_SECOND); |
There was a problem hiding this comment.
We can optimize this by avoiding the second division/modulo operation (Math.floorMod). Since we already have pgSeconds (the result of Math.floorDiv), we can compute the remainder using multiplication and subtraction, which is significantly faster.
| long remainingMicroseconds = Math.floorMod(pgMicroseconds, MICROSECONDS_IN_SECOND); | |
| long remainingMicroseconds = pgMicroseconds - pgSeconds * MICROSECONDS_IN_SECOND; |
There was a problem hiding this comment.
No, it is not worth the added cognitive complexity (and significantly faster is maybe true in relative sense, but we are talking about nanoseconds here...)
In PostgreSQL's binary wire protocol, timestamps are encoded as microseconds relative to 2000-01-01 (the PostgreSQL epoch). Previously, TimestampParser.toTimestamp(byte[]) passed the raw microsecond value directly to Timestamp.ofTimeMicroseconds(pgMicros) before adding the 30-year epoch offset (PG_EPOCH_SECONDS). Because Timestamp.ofTimeMicroseconds checks bounds against the 1970 Unix epoch, any date between 0001-01-01 and ~0031-01-01 was treated as falling before the minimum supported timestamp, throwing an uncaught IllegalArgumentException (SQLState P0001).
This change: