Skip to content

Commit 46f8aed

Browse files
def-claude
andcommitted
repr, expr: roll leap-second timestamps into the next minute
chrono represents a parsed :60 second as 59 with nanos >= 1e9, a value that sorts before the following second while the epoch family (extract epoch, casts to mz_timestamp, timestamp subtraction, date_bin, precision rounding) counts it at or past that second. That breaks the monotonicity contracts persist filter pushdown narrows ranges with: an interior leap value escapes the endpoint-derived range, so a part holding one could be wrongly discarded. Roll :60 over at the parse boundary, matching Postgres, so the leap representation never enters a timestamp. TIME keeps it: rolling a time over would wrap to 00:00:00 and reverse its ordering, and the whole-second leap time is provably harmless (fractional leap seconds are rejected at parse for all types). add_date_time is the other way a leap representation reached a timestamp. It passed a leap-second TIME's raw nanos straight through, so DATE + TIME '23:59:60' still constructed one and disagreed with the equivalent timestamp literal. It now rolls over the same way. Both rollovers can overflow chrono's range: TIMESTAMP '262142-12-31 23:59:60' parses, because that is chrono's max date and the leap value passes the HIGH_DATE check, and adding the rollover second overflows. Overflow reports out of range in the timestamp, timestamptz, and DATE + TIME paths instead of panicking. Closes: PER-63 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent bb4a614 commit 46f8aed

3 files changed

Lines changed: 84 additions & 5 deletions

File tree

‎src/expr/src/scalar/func.rs‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -344,9 +344,20 @@ fn add_date_time(
344344
date: Date,
345345
time: chrono::NaiveTime,
346346
) -> Result<CheckedTimestamp<NaiveDateTime>, EvalError> {
347+
// A leap-second TIME (nanos >= 1e9) rolls over into the next minute,
348+
// matching what parsing the equivalent timestamp literal produces. The
349+
// leap representation must not enter a timestamp: it sorts before the
350+
// next second while epoch-style conversions count it at or past it,
351+
// breaking the monotonicity contracts filter pushdown relies on.
352+
let (extra_sec, nanos) = match time.nanosecond().checked_sub(1_000_000_000) {
353+
Some(nanos) => (1, nanos),
354+
None => (0, time.nanosecond()),
355+
};
347356
let dt = NaiveDate::from(date)
348-
.and_hms_nano_opt(time.hour(), time.minute(), time.second(), time.nanosecond())
349-
.unwrap();
357+
.and_hms_nano_opt(time.hour(), time.minute(), time.second(), nanos)
358+
.unwrap()
359+
.checked_add_signed(chrono::Duration::try_seconds(extra_sec).unwrap())
360+
.ok_or(EvalError::TimestampOutOfRange)?;
350361
Ok(CheckedTimestamp::from_timestamplike(dt)?)
351362
}
352363

‎src/repr/src/strconv.rs‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -525,12 +525,39 @@ fn parse_timestamp_inner(
525525
order: DateOrder,
526526
) -> Result<CheckedTimestamp<NaiveDateTime>, ParseError> {
527527
match parse_timestamp_string(s, order) {
528-
Ok((date, time, _)) => CheckedTimestamp::from_timestamplike(date.and_time(time))
529-
.map_err(|_| ParseError::out_of_range("timestamp", s)),
528+
Ok((date, time, _)) => roll_over_leap_second(date.and_time(time))
529+
.ok_or_else(|| ParseError::out_of_range("timestamp", s))
530+
.and_then(|dt| {
531+
CheckedTimestamp::from_timestamplike(dt)
532+
.map_err(|_| ParseError::out_of_range("timestamp", s))
533+
}),
530534
Err(e) => Err(ParseError::invalid_input_syntax("timestamp", s).with_details(e)),
531535
}
532536
}
533537

538+
/// Postgres normalizes a parsed `:60` second by rolling it into the next
539+
/// minute. chrono instead keeps a leap-second representation (nanos >= 1e9)
540+
/// that sorts before the following second while epoch-style conversions
541+
/// count it at or past that second, which breaks the monotonicity contracts
542+
/// persist filter pushdown derives ranges with. Roll it over at the parse
543+
/// boundary so the leap representation never enters a parsed timestamp.
544+
/// `TIME` keeps the leap representation: rolling it over would wrap to
545+
/// 00:00:00 and reverse its ordering, and the whole-second leap time is
546+
/// harmless.
547+
///
548+
/// Returns `None` when the rollover overflows chrono's range (a leap second
549+
/// on the maximum date), which callers report as out of range.
550+
fn roll_over_leap_second(dt: NaiveDateTime) -> Option<NaiveDateTime> {
551+
use chrono::Timelike;
552+
match dt.nanosecond().checked_sub(1_000_000_000) {
553+
Some(nanos) => dt
554+
.with_nanosecond(nanos)
555+
.expect("in range")
556+
.checked_add_signed(Duration::try_seconds(1).unwrap()),
557+
None => Some(dt),
558+
}
559+
}
560+
534561
/// Writes a [`NaiveDateTime`] timestamp to `buf`.
535562
pub fn format_timestamp<F>(buf: &mut F, ts: &NaiveDateTime) -> Nestable
536563
where
@@ -569,7 +596,8 @@ fn parse_timestamptz_inner(
569596
parse_timestamp_string(s, order)
570597
.and_then(|(date, time, timezone)| {
571598
use Timezone::*;
572-
let mut dt = date.and_time(time);
599+
let mut dt = roll_over_leap_second(date.and_time(time))
600+
.ok_or_else(|| "timestamp out of range".to_owned())?;
573601
let offset = match timezone {
574602
FixedOffset(offset) => offset,
575603
Tz(tz) => match tz.offset_from_local_datetime(&dt).latest() {
@@ -2236,6 +2264,17 @@ mod tests {
22362264

22372265
use super::*;
22382266

2267+
/// Rolling a leap second over must not panic at the timestamp maximum:
2268+
/// chrono's max date parses, and adding the rollover second overflows.
2269+
/// The leap value on the max date errors as out of range instead.
2270+
#[mz_ore::test]
2271+
fn leap_second_rollover_at_max_date_errors() {
2272+
assert!(parse_timestamp("262142-12-31 23:59:60").is_err());
2273+
assert!(parse_timestamptz("262142-12-31 23:59:60+00").is_err());
2274+
// One second below the maximum still rolls over successfully.
2275+
assert_ok!(parse_timestamp("262142-12-31 23:59:59"));
2276+
}
2277+
22392278
proptest! {
22402279
#[mz_ore::test]
22412280
#[cfg_attr(miri, ignore)] // too slow

‎test/sqllogictest/explain/pushdown.slt‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,35 @@ ALTER SYSTEM RESET persist_stats_audit_percent
185185
----
186186
COMPLETE 0
187187

188+
# Leap-second timestamps roll over at parse time (Postgres compat). The
189+
# chrono leap representation sorted below the next second while the epoch
190+
# family (extract epoch, casts to mz_timestamp, subtraction, date_bin)
191+
# counted it at or past that second, breaking the monotonicity contracts
192+
# pushdown narrows ranges with.
193+
194+
query TT
195+
SELECT TIMESTAMP '2015-06-30 23:59:60' = TIMESTAMP '2015-07-01 00:00:00', TIMESTAMPTZ '2015-06-30 23:59:60+00' = TIMESTAMPTZ '2015-07-01 00:00:00+00'
196+
----
197+
true true
198+
199+
statement ok
200+
CREATE TABLE leap (ts timestamp)
201+
202+
statement ok
203+
INSERT INTO leap VALUES ('2015-06-30 23:59:59.4'), ('2015-06-30 23:59:60'), ('2015-07-01 00:00:00.2')
204+
205+
query I
206+
SELECT count(*) FROM leap WHERE ts::mz_timestamp = 1435708800000::mz_timestamp
207+
----
208+
1
209+
210+
# DATE + TIME rolls a leap second over the same way the timestamp literal
211+
# does, so the two spellings agree.
212+
query T
213+
SELECT DATE '2015-06-30' + TIME '23:59:60' = TIMESTAMP '2015-07-01 00:00:00'
214+
----
215+
true
216+
188217
# EXPLAIN FILTER PUSHDOWN FOR MATERIALIZED VIEW is also supported
189218

190219
statement ok

0 commit comments

Comments
 (0)