From cd319927ce6c497f116042cdc6b3b567cc39b6aa Mon Sep 17 00:00:00 2001 From: gitRasheed <99349713+gitRasheed@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:18:47 +0100 Subject: [PATCH 1/4] fix(array): accept timestamps beyond jiff's range Timestamp scalars were validated against `jiff::Timestamp`'s range, which stops 26 hours short of the end of year 9999, so a value Arrow, Parquet and Spark allow, such as `9999-12-31T00:00:00Z`, could neither be used as a literal nor written: the file statistics built the min/max scalar with `Scalar::extension_ref` and panicked. Every `i64` count of the unit is a valid timestamp; jiff only bounds what `Display` can render, so drop the range check, fall back to the raw count when the calendar rendering is unavailable, and build the min/max scalars fallibly so a rejected extreme is an error rather than a panic. Signed-off-by: gitRasheed <99349713+gitRasheed@users.noreply.github.com> --- .../src/aggregate_fn/fns/min_max/extension.rs | 19 +++-- .../src/aggregate_fn/fns/min_max/mod.rs | 26 ++++++ .../src/extension/datetime/timestamp.rs | 81 +++++++++++-------- 3 files changed, 83 insertions(+), 43 deletions(-) diff --git a/vortex-array/src/aggregate_fn/fns/min_max/extension.rs b/vortex-array/src/aggregate_fn/fns/min_max/extension.rs index 4c18fad0e6d..658d374604d 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/extension.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/extension.rs @@ -11,6 +11,7 @@ use crate::aggregate_fn::AggregateArgs; use crate::aggregate_fn::NumericalAggregateOpts; use crate::arrays::ExtensionArray; use crate::arrays::extension::ExtensionArrayExt; +use crate::dtype::DType; use crate::dtype::Nullability; use crate::scalar::Scalar; @@ -20,15 +21,17 @@ pub(super) fn accumulate_extension( array: &ExtensionArray, ctx: &mut ExecutionCtx, ) -> VortexResult<()> { - let non_nullable_ext_dtype = array.ext_dtype().with_nullability(Nullability::NonNullable); + let dtype = DType::Extension(array.ext_dtype().with_nullability(Nullability::NonNullable)); // A NaN in float storage comes back as NaN extrema, which `merge` skips or poisons with. - let local = - min_max(array.storage_array(), ctx, *args.options)?.map(|MinMaxResult { min, max }| { - MinMaxResult { - min: Scalar::extension_ref(non_nullable_ext_dtype.clone(), min), - max: Scalar::extension_ref(non_nullable_ext_dtype, max), - } - }); + let local = min_max(array.storage_array(), ctx, *args.options)? + .map(|MinMaxResult { min, max }| { + // Storage extremes the extension type rejects are an error, not an invariant violation. + VortexResult::Ok(MinMaxResult { + min: Scalar::try_new(dtype.clone(), min.into_value())?, + max: Scalar::try_new(dtype, max.into_value())?, + }) + }) + .transpose()?; partial.merge(args, local); Ok(()) } diff --git a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs index ce3e938a8fd..e04f2b05cbf 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs @@ -567,6 +567,7 @@ mod tests { use crate::arrays::ListArray; use crate::arrays::NullArray; use crate::arrays::PrimitiveArray; + use crate::arrays::TemporalArray; use crate::arrays::VarBinArray; use crate::dtype::BigCast; use crate::dtype::DType; @@ -580,6 +581,7 @@ mod tests { use crate::expr::stats::Precision; use crate::expr::stats::Stat; use crate::expr::stats::StatsProvider; + use crate::extension::datetime::TimeUnit; use crate::match_each_decimal_value_type; use crate::scalar::DecimalValue; use crate::scalar::Scalar; @@ -602,6 +604,30 @@ mod tests { Ok(()) } + /// A timestamp on the last day of year 9999 lies beyond `jiff`'s range but is a valid Arrow + /// and Parquet value, so the statistics of a column holding one must not fail. + #[test] + fn test_timestamp_min_max_beyond_jiff_range() -> VortexResult<()> { + // 9999-12-31T00:00:00Z + const END_OF_9999_US: i64 = 253_402_214_400_000_000; + let timestamps = TemporalArray::new_timestamp( + PrimitiveArray::from_iter([0i64, END_OF_9999_US]).into_array(), + TimeUnit::Microseconds, + None, + ) + .into_array(); + let mut ctx = SESSION.create_execution_ctx(); + let MinMaxResult { min, max } = + min_max(×tamps, &mut ctx, NumericalAggregateOpts::default())? + .vortex_expect("non-empty array"); + assert_eq!(min.as_extension().to_storage_scalar(), 0i64.into()); + assert_eq!( + max.as_extension().to_storage_scalar(), + END_OF_9999_US.into() + ); + Ok(()) + } + #[test] fn test_prim_min_max_multiple_null_runs() -> VortexResult<()> { // Several disjoint valid runs separated by nulls exercise the per-run fold; the extrema diff --git a/vortex-array/src/extension/datetime/timestamp.rs b/vortex-array/src/extension/datetime/timestamp.rs index 42f00776216..10bc5eee9c4 100644 --- a/vortex-array/src/extension/datetime/timestamp.rs +++ b/vortex-array/src/extension/datetime/timestamp.rs @@ -6,7 +6,6 @@ use std::fmt; use std::sync::Arc; -use jiff::Span; use jiff::fmt::temporal::DateTimeParser; use jiff::tz::TimeZone; use vortex_error::VortexExpect; @@ -104,13 +103,20 @@ pub enum TimestampValue<'a> { impl fmt::Display for TimestampValue<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let (span, tz) = match self { - TimestampValue::Seconds(v, tz) => (Span::new().seconds(*v), *tz), - TimestampValue::Milliseconds(v, tz) => (Span::new().milliseconds(*v), *tz), - TimestampValue::Microseconds(v, tz) => (Span::new().microseconds(*v), *tz), - TimestampValue::Nanoseconds(v, tz) => (Span::new().nanoseconds(*v), *tz), + let (unit, value, tz) = match *self { + TimestampValue::Seconds(v, tz) => (TimeUnit::Seconds, v, tz), + TimestampValue::Milliseconds(v, tz) => (TimeUnit::Milliseconds, v, tz), + TimestampValue::Microseconds(v, tz) => (TimeUnit::Microseconds, v, tz), + TimestampValue::Nanoseconds(v, tz) => (TimeUnit::Nanoseconds, v, tz), + }; + // `jiff` stops 26 hours short of the end of years 9999 and -9999, so a valid timestamp may + // have no calendar rendering; show the count then. + let Ok(ts) = unit + .to_jiff_span(value) + .and_then(|span| Ok(jiff::Timestamp::UNIX_EPOCH.checked_add(span)?)) + else { + return write!(f, "{value}{unit}"); }; - let ts = jiff::Timestamp::UNIX_EPOCH + span; match tz { None => write!(f, "{ts}"), @@ -211,39 +217,20 @@ impl ExtVTable for Timestamp { let ts_value = storage_value.as_primitive().cast::()?; let tz = metadata.tz.as_ref(); - let (span, value) = match metadata.unit { - TimeUnit::Nanoseconds => ( - Span::new().nanoseconds(ts_value), - TimestampValue::Nanoseconds(ts_value, tz), - ), - TimeUnit::Microseconds => ( - Span::new().microseconds(ts_value), - TimestampValue::Microseconds(ts_value, tz), - ), - TimeUnit::Milliseconds => ( - Span::new().milliseconds(ts_value), - TimestampValue::Milliseconds(ts_value, tz), - ), - TimeUnit::Seconds => ( - Span::new().seconds(ts_value), - TimestampValue::Seconds(ts_value, tz), - ), - TimeUnit::Days => { - vortex_bail!("Timestamp does not support Days time unit") - } - }; - - // Validate the storage value is within the valid range for Timestamp. - jiff::Timestamp::UNIX_EPOCH - .checked_add(span) - .map_err(|e| vortex_err!("Invalid timestamp scalar: {}", e))?; - if let Some(tz) = tz { resolve_time_zone(Some(tz)) .map_err(|e| e.with_context("Invalid timezone for timestamp scalar"))?; } - Ok(value) + // Every `i64` count of the unit is a valid timestamp, as in Arrow and Parquet. `jiff`'s + // narrower range only bounds what `Display` can render as a calendar date. + Ok(match metadata.unit { + TimeUnit::Nanoseconds => TimestampValue::Nanoseconds(ts_value, tz), + TimeUnit::Microseconds => TimestampValue::Microseconds(ts_value, tz), + TimeUnit::Milliseconds => TimestampValue::Milliseconds(ts_value, tz), + TimeUnit::Seconds => TimestampValue::Seconds(ts_value, tz), + TimeUnit::Days => vortex_bail!("Timestamp does not support Days time unit"), + }) } } @@ -270,6 +257,30 @@ mod tests { Ok(()) } + /// Arrow and Parquet store every `i64` count of the unit, so a timestamp on the last day of + /// year 9999, beyond `jiff`'s range, is a valid scalar; it displays as the raw count. + #[cfg_attr(miri, ignore)] + #[rstest] + #[case::end_of_9999( + TimeUnit::Microseconds, + 253_402_214_400_000_000, + "253402214400000000µs" + )] + #[case::start_of_neg_9999(TimeUnit::Seconds, -377_705_116_800, "-377705116800s")] + #[case::max(TimeUnit::Microseconds, i64::MAX, "9223372036854775807µs")] + fn accept_timestamp_beyond_jiff_range( + #[case] unit: TimeUnit, + #[case] value: i64, + #[case] expected: &str, + ) -> VortexResult<()> { + for tz in [None, Some(Arc::from("UTC"))] { + let dtype = DType::Extension(Timestamp::new_with_tz(unit, tz, Nullable).erased()); + let scalar = Scalar::try_new(dtype, Some(ScalarValue::Primitive(PValue::I64(value))))?; + assert_eq!(format!("{}", scalar.as_extension()), expected); + } + Ok(()) + } + #[cfg_attr(miri, ignore)] #[rstest] #[case("+01:00", "1970-01-01T01:00:00+01:00[+01:00]")] From c9a4860cd6df75cae0dd671fad9c9973772fbea7 Mon Sep 17 00:00:00 2001 From: gitRasheed <99349713+gitRasheed@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:18:48 +0100 Subject: [PATCH 2/4] fix(datetime-parts): reject timestamps whose days do not fit i32 Now that timestamps beyond jiff's range can be written, `split_temporal` could see second and millisecond counts more than i32::MAX days from the epoch. `split_with_divisor` truncated the days to i32 with `as_()`, so such values silently wrapped when compressed: `i64::MAX` seconds round-tripped as 50027779063807. The compress comment still claimed jiff validation made the narrowing safe, the btrblocks TemporalScheme applied the encoding with no range check, and `try_extract_days_constant` used the same truncating split, so `dtp <= lit` with such a literal was rewritten to a wrapped days constant. Microsecond and nanosecond counts cannot overflow the days part. `timestamp::split` now errors when the days do not fit, so the days pushdown declines the constant and the compare kernel falls back to the canonical comparison. `split_temporal` checks the valid min/max of second and millisecond arrays and errors instead of wrapping, and the new `can_split_temporal` lets the btrblocks TemporalScheme skip such arrays so the storage array is compressed and the data round-trips exactly. The narrowing compare test built its fixture from `i64::MAX` seconds and relied on the wrap; it now uses a value above i32::MAX whose days fit. Signed-off-by: gitRasheed <99349713+gitRasheed@users.noreply.github.com> --- encodings/datetime-parts/src/compress.rs | 85 ++++++++++++++++++- .../datetime-parts/src/compute/compare.rs | 10 ++- encodings/datetime-parts/src/compute/rules.rs | 7 ++ encodings/datetime-parts/src/timestamp.rs | 49 ++++++++++- vortex-btrblocks/src/schemes/temporal.rs | 16 +++- vortex-btrblocks/src/tests.rs | 24 ++++++ 6 files changed, 177 insertions(+), 14 deletions(-) diff --git a/encodings/datetime-parts/src/compress.rs b/encodings/datetime-parts/src/compress.rs index d84f4b6be91..2d1caecacfb 100644 --- a/encodings/datetime-parts/src/compress.rs +++ b/encodings/datetime-parts/src/compress.rs @@ -6,6 +6,9 @@ use std::mem::MaybeUninit; use vortex_array::ArrayRef; use vortex_array::ExecutionCtx; use vortex_array::IntoArray; +use vortex_array::aggregate_fn::NumericalAggregateOpts; +use vortex_array::aggregate_fn::fns::min_max::MinMaxResult; +use vortex_array::aggregate_fn::fns::min_max::min_max; use vortex_array::arrays::ConstantArray; use vortex_array::arrays::PrimitiveArray; use vortex_array::arrays::TemporalArray; @@ -18,9 +21,10 @@ use vortex_error::VortexResult; use vortex_error::vortex_bail; use crate::timestamp; -/// All parts are stored as i32. seconds and subseconds always fit in i32. -/// For days we have a upper day limitation due to "jiff" validation so days -/// also fit in i32. + +/// All parts are stored as i32. Seconds and subseconds always fit in i32. Days are checked by +/// [`split_temporal`]: second and millisecond counts can lie more than `i32::MAX` days from the +/// epoch, and such arrays are rejected rather than wrapped. pub struct TemporalParts { pub days: ArrayRef, pub seconds: ArrayRef, @@ -31,11 +35,22 @@ pub struct TemporalParts { /// /// Splitting the components by granularity creates more small values, which enables better /// cascading compression. +/// +/// # Errors +/// +/// Returns an error if the time unit is days, or if a valid timestamp lies more than `i32::MAX` +/// days from the epoch (see [`can_split_temporal`]). pub fn split_temporal(array: TemporalArray, ctx: &mut ExecutionCtx) -> VortexResult { let time_unit = array.temporal_metadata().time_unit(); if matches!(time_unit, TimeUnit::Days) { vortex_bail!("Cannot handle day-level data"); } + if !days_fit(array.temporal_values(), time_unit, ctx)? { + vortex_bail!( + "{time_unit} timestamps lie more than i32::MAX days from the epoch and cannot be \ + split into days" + ); + } let temporal_values = array .temporal_values() @@ -109,6 +124,33 @@ pub fn split_temporal(array: TemporalArray, ctx: &mut ExecutionCtx) -> VortexRes }) } +/// Whether `array` can be split into [`TemporalParts`]: every valid timestamp's days since the +/// epoch fit the `i32` days part. Microsecond and nanosecond timestamps always can. +pub fn can_split_temporal(array: &TemporalArray, ctx: &mut ExecutionCtx) -> VortexResult { + let time_unit = array.temporal_metadata().time_unit(); + if matches!(time_unit, TimeUnit::Days) { + return Ok(false); + } + days_fit(array.temporal_values(), time_unit, ctx) +} + +/// Checks the days of the valid extremes of `values`, which bound every other valid value. +fn days_fit(values: &ArrayRef, time_unit: TimeUnit, ctx: &mut ExecutionCtx) -> VortexResult { + if matches!(time_unit, TimeUnit::Microseconds | TimeUnit::Nanoseconds) { + return Ok(true); + } + let Some(MinMaxResult { min, max }) = min_max(values, ctx, NumericalAggregateOpts::default())? + else { + return Ok(true); + }; + Ok([min, max].iter().all(|bound| { + bound + .as_primitive() + .as_::() + .is_some_and(|ts| timestamp::days_fit(ts, time_unit)) + })) +} + #[inline] fn split_slice( days: &mut [MaybeUninit], @@ -177,13 +219,17 @@ mod tests { use vortex_array::VortexSessionExecute; use vortex_array::arrays::PrimitiveArray; use vortex_array::arrays::TemporalArray; + use vortex_array::assert_arrays_eq; use vortex_array::extension::datetime::TimeUnit; use vortex_array::validity::Validity; use vortex_buffer::buffer; use vortex_error::VortexExpect; + use vortex_error::VortexResult; use vortex_session::VortexSession; + use crate::DateTimeParts; use crate::TemporalParts; + use crate::can_split_temporal; use crate::split_temporal; static SESSION: LazyLock = LazyLock::new(VortexSession::empty); @@ -235,4 +281,37 @@ mod tests { Validity::NonNullable )); } + + /// Extreme timestamps either split exactly or are rejected, never wrapped: `i64::MAX` seconds + /// used to come back as 50027779063807. + #[rstest] + #[case::seconds(TimeUnit::Seconds, false)] + #[case::milliseconds(TimeUnit::Milliseconds, false)] + #[case::microseconds(TimeUnit::Microseconds, true)] + #[case::nanoseconds(TimeUnit::Nanoseconds, true)] + fn extreme_timestamps_split_exactly_or_fail( + #[case] unit: TimeUnit, + #[case] splittable: bool, + ) -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let temporal = TemporalArray::new_timestamp( + PrimitiveArray::from_option_iter([Some(i64::MAX), None, Some(i64::MIN), Some(0)]) + .into_array(), + unit, + Some("UTC".into()), + ); + assert_eq!(can_split_temporal(&temporal, &mut ctx)?, splittable); + + match DateTimeParts::try_from_temporal(temporal.clone(), &mut ctx) { + Ok(parts) => { + assert_arrays_eq!(parts, temporal, &mut ctx); + assert!(splittable, "{unit} must be rejected"); + } + Err(err) => { + assert!(!splittable, "{unit} must split: {err}"); + assert!(err.to_string().contains("i32::MAX days"), "{err}"); + } + } + Ok(()) + } } diff --git a/encodings/datetime-parts/src/compute/compare.rs b/encodings/datetime-parts/src/compute/compare.rs index 96885800424..c6fa15a96a1 100644 --- a/encodings/datetime-parts/src/compute/compare.rs +++ b/encodings/datetime-parts/src/compute/compare.rs @@ -52,7 +52,11 @@ impl CompareKernel for DateTimeParts { let Some(options) = ext_dtype.metadata_opt::() else { return Ok(None); }; - let ts_parts = timestamp::split(timestamp, options.unit)?; + // A constant more than i32::MAX days from the epoch has no days part to compare with: + // leave it to the canonical comparison. + let Ok(ts_parts) = timestamp::split(timestamp, options.unit) else { + return Ok(None); + }; let result = match operator { CompareOperator::Eq => compare_eq(lhs, &ts_parts, nullability, ctx)?, @@ -443,8 +447,8 @@ mod test { ) .unwrap(); - // Timestamp with a value larger than i32::MAX. - let rhs = dtp_array_from_timestamp(i64::MAX, rhs_validity); + // Timestamp with a value larger than i32::MAX, whose days still fit the i32 days part. + let rhs = dtp_array_from_timestamp(i64::from(i32::MAX) + 1, rhs_validity); let comparison = lhs .clone() diff --git a/encodings/datetime-parts/src/compute/rules.rs b/encodings/datetime-parts/src/compute/rules.rs index 7ebe8477b00..b251a7d925e 100644 --- a/encodings/datetime-parts/src/compute/rules.rs +++ b/encodings/datetime-parts/src/compute/rules.rs @@ -278,6 +278,13 @@ mod tests { ConstantArray::new(scalar, len).into_array() } + /// A midnight constant whose days do not fit `i32` must not be pushed down as wrapped days. + #[test] + fn days_pushdown_declines_constant_outside_i32_days() { + let constant = midnight_constant(i64::from(i32::MAX) + 1, TimeUnit::Seconds, 1); + assert_eq!(try_extract_days_constant(&constant), None); + } + #[test] fn test_midnight_binary_comparison() { // DTP with days [0, 1, 2] at midnight diff --git a/encodings/datetime-parts/src/timestamp.rs b/encodings/datetime-parts/src/timestamp.rs index a9e9bcc3322..3395d75ec44 100644 --- a/encodings/datetime-parts/src/timestamp.rs +++ b/encodings/datetime-parts/src/timestamp.rs @@ -12,7 +12,7 @@ pub const SECONDS_PER_DAY: i64 = 86_400; // 24 * 60 * 60 /// Parts of a Unix timestamp (time since 1970-01-01 00:00:00 UTC). /// /// Broken down into: -/// * Days since epoch (max range is "jiff"'s range) +/// * Days since epoch (must fit `i32`, see [`days_fit`]) /// * Seconds within day (0-86399) /// * Subseconds (range depends on `TimeUnit`) pub struct TimestampParts { @@ -30,18 +30,44 @@ pub struct TimestampParts { /// /// # Errors /// -/// Returns an error if `time_unit` is days, which cannot be split. +/// Returns an error if `time_unit` is days, which cannot be split, or if the timestamp's days +/// since the epoch do not fit the `i32` days part. pub fn split(timestamp: i64, time_unit: TimeUnit) -> VortexResult { - Ok(match time_unit { + let parts = match time_unit { TimeUnit::Nanoseconds => split_with_divisor::<1_000_000_000>(timestamp), TimeUnit::Microseconds => split_with_divisor::<1_000_000>(timestamp), TimeUnit::Milliseconds => split_with_divisor::<1_000>(timestamp), TimeUnit::Seconds => split_with_divisor::<1>(timestamp), TimeUnit::Days => vortex_bail!("Cannot handle day-level data"), - }) + }; + if !days_fit(timestamp, time_unit) { + vortex_bail!( + "Timestamp {timestamp}{time_unit} lies more than i32::MAX days from the epoch and \ + cannot be split into days" + ); + } + Ok(parts) +} + +/// Whether the days since the epoch of `timestamp` fit the `i32` days part of [`TimestampParts`]. +/// +/// Every `i64` count of microseconds or nanoseconds fits. Second and millisecond counts can lie +/// further from the epoch than `i32::MAX` days. +pub fn days_fit(timestamp: i64, time_unit: TimeUnit) -> bool { + let ticks_per_second = match time_unit { + TimeUnit::Nanoseconds => 1_000_000_000, + TimeUnit::Microseconds => 1_000_000, + TimeUnit::Milliseconds => 1_000, + TimeUnit::Seconds => 1, + TimeUnit::Days => return false, + }; + i32::try_from(timestamp / (SECONDS_PER_DAY * ticks_per_second)).is_ok() } /// Split a Unix timestamp into parts using a constant DIVISOR. +/// +/// Days that do not fit `i32` wrap: callers check [`days_fit`] first, as [`split`] does for a +/// single timestamp and `split_temporal` does for a whole array. #[inline] pub(crate) fn split_with_divisor(timestamp: i64) -> TimestampParts { let days = timestamp / (SECONDS_PER_DAY * DIVISOR); @@ -161,6 +187,21 @@ mod tests { assert!(split(1, TimeUnit::Days).is_err()); } + #[rstest] + #[case::seconds(TimeUnit::Seconds, false)] + #[case::milliseconds(TimeUnit::Milliseconds, false)] + #[case::microseconds(TimeUnit::Microseconds, true)] + #[case::nanoseconds(TimeUnit::Nanoseconds, true)] + fn split_rejects_days_outside_i32(#[case] unit: TimeUnit, #[case] fits: bool) { + for ts in [i64::MAX, i64::MIN] { + assert_eq!(days_fit(ts, unit), fits, "{ts}{unit}"); + match split(ts, unit) { + Ok(parts) => assert_eq!(combine(parts, unit), ts, "{ts}{unit}"), + Err(err) => assert!(!fits && err.to_string().contains("i32::MAX days"), "{err}"), + } + } + } + #[test] fn test_combine_seconds() { // 1970-01-02 01:02:03 UTC diff --git a/vortex-btrblocks/src/schemes/temporal.rs b/vortex-btrblocks/src/schemes/temporal.rs index 79748b69450..628e42522fe 100644 --- a/vortex-btrblocks/src/schemes/temporal.rs +++ b/vortex-btrblocks/src/schemes/temporal.rs @@ -21,6 +21,7 @@ use vortex_compressor::scheme::CompressionEstimate; use vortex_compressor::scheme::EstimateVerdict; use vortex_datetime_parts::DateTimeParts; use vortex_datetime_parts::TemporalParts; +use vortex_datetime_parts::can_split_temporal; use vortex_datetime_parts::split_temporal; use vortex_error::VortexResult; @@ -66,12 +67,19 @@ impl Scheme for TemporalScheme { fn expected_compression_ratio( &self, - _data: &ArrayAndStats, + data: &ArrayAndStats, _compress_ctx: CompressorContext, - _exec_ctx: &mut ExecutionCtx, + exec_ctx: &mut ExecutionCtx, ) -> CompressionEstimate { - // Temporal compression (splitting into parts) is almost always beneficial. - CompressionEstimate::Verdict(EstimateVerdict::AlwaysUse) + // Temporal compression (splitting into parts) is almost always beneficial, but second and + // millisecond timestamps more than i32::MAX days from the epoch have no i32 days part, so + // `split_temporal` rejects them and the storage array is compressed instead. + let splittable = TemporalArray::try_from(data.array().clone()) + .and_then(|temporal| can_split_temporal(&temporal, exec_ctx)); + match splittable { + Ok(true) => CompressionEstimate::Verdict(EstimateVerdict::AlwaysUse), + _ => CompressionEstimate::Verdict(EstimateVerdict::Skip), + } } fn compress( diff --git a/vortex-btrblocks/src/tests.rs b/vortex-btrblocks/src/tests.rs index 03250fc4491..f0843d93cec 100644 --- a/vortex-btrblocks/src/tests.rs +++ b/vortex-btrblocks/src/tests.rs @@ -32,6 +32,7 @@ use vortex_array::dtype::DecimalDType; use vortex_array::dtype::Nullability; use vortex_array::extension::datetime::TimeUnit; use vortex_array::validity::Validity; +use vortex_datetime_parts::DateTimeParts; #[cfg(feature = "zstd")] use vortex_edition::EDITION_DECLARATIONS; #[cfg(feature = "zstd")] @@ -111,6 +112,29 @@ fn test_default_compressor_roundtrip() -> VortexResult<()> { Ok(()) } +/// Second timestamps beyond i32::MAX days from the epoch have no DateTimeParts form; the +/// compressor must fall back to the storage rather than write wrapped days. +#[test] +fn test_extreme_second_timestamps_roundtrip() -> VortexResult<()> { + let compressor = BtrBlocksCompressorBuilder::from_session(&SESSION) + .unrestricted() + .build(); + let input = TemporalArray::new_timestamp( + PrimitiveArray::from_iter((0..2048i64).map(|i| match i % 3 { + 0 => i64::MAX - i, + 1 => i64::MIN + i, + _ => 1_700_000_000 + i, + })) + .into_array(), + TimeUnit::Seconds, + None, + ) + .into_array(); + let compressed = assert_roundtrip(&compressor, &input)?; + assert!(!compressed.is::()); + Ok(()) +} + #[rstest] #[case::unaligned({ let mut rng = StdRng::seed_from_u64(7); From ec6270e36abc0178ce0195c06dc13a9a06d8927b Mon Sep 17 00:00:00 2001 From: gitRasheed <99349713+gitRasheed@users.noreply.github.com> Date: Fri, 23 Oct 2026 19:40:56 +0100 Subject: [PATCH 3/4] fix(datetime-parts): decline days pushdown when narrowed days cannot hold the literal Compression narrows the days part, for example to u16 for dates before 2149. A midnight literal past that range, such as 9999-12-31, made the comparison push-down cast its days to the narrowed type and fail with "Cannot cast i32 to u16". The rule now declines, and the comparison runs on the full timestamps. Signed-off-by: gitRasheed <99349713+gitRasheed@users.noreply.github.com> --- encodings/datetime-parts/src/compute/rules.rs | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/encodings/datetime-parts/src/compute/rules.rs b/encodings/datetime-parts/src/compute/rules.rs index b251a7d925e..078c91da45e 100644 --- a/encodings/datetime-parts/src/compute/rules.rs +++ b/encodings/datetime-parts/src/compute/rules.rs @@ -13,12 +13,12 @@ use vortex_array::arrays::scalar_fn::AnyScalarFn; use vortex_array::arrays::scalar_fn::ScalarFn; use vortex_array::arrays::scalar_fn::ScalarFnArrayExt; use vortex_array::arrays::slice::SliceReduceAdaptor; -use vortex_array::builtins::ArrayBuiltins; use vortex_array::dtype::DType; use vortex_array::extension::datetime::Timestamp; use vortex_array::optimizer::ArrayOptimizer; use vortex_array::optimizer::rules::ArrayParentReduceRule; use vortex_array::optimizer::rules::ParentRuleSet; +use vortex_array::scalar::Scalar; use vortex_array::scalar_fn::fns::between::Between; use vortex_array::scalar_fn::fns::binary::Binary; use vortex_array::scalar_fn::fns::cast::CastReduceAdaptor; @@ -138,8 +138,11 @@ impl ArrayParentReduceRule for DTPComparisonPushDownRule { // Keep the nullability of the replaced constant, so the rewritten comparison has // the same result dtype as the original. let target_dtype = days.dtype().with_nullability(c.dtype().nullability()); - let constant = ConstantArray::new(days_value, len).into_array(); - new_children.push(constant.cast(target_dtype)?); + // Narrowed days, such as u16, cannot hold every literal's days. + let Ok(days_scalar) = Scalar::from(days_value).cast(&target_dtype) else { + return Ok(None); + }; + new_children.push(ConstantArray::new(days_scalar, len).into_array()); } } @@ -190,6 +193,7 @@ fn is_constant_zero(array: &ArrayRef) -> bool { mod tests { use std::sync::LazyLock; + use rstest::rstest; use vortex_array::ArrayRef; use vortex_array::VortexSessionExecute; use vortex_array::arrays::BoolArray; @@ -285,6 +289,37 @@ mod tests { assert_eq!(try_extract_days_constant(&constant), None); } + /// Days compressed to u16 cannot hold 9999-12-31, so the comparison is not pushed down. + #[rstest] + #[case::lt(Operator::Lt, true)] + #[case::lte(Operator::Lte, true)] + #[case::eq(Operator::Eq, false)] + #[case::not_eq(Operator::NotEq, true)] + #[case::gt(Operator::Gt, false)] + fn test_midnight_comparison_beyond_narrowed_days(#[case] op: Operator, #[case] expected: bool) { + let dtype = dtp_at_midnight(&[0], TimeUnit::Microseconds) + .dtype() + .clone(); + let narrowed = DateTimeParts::try_new( + dtype, + PrimitiveArray::from_iter([20_000u16, 20_001]).into_array(), + ConstantArray::new(0u8, 2).into_array(), + ConstantArray::new(0u8, 2).into_array(), + ) + .unwrap(); + let constant = midnight_constant(2_932_896, TimeUnit::Microseconds, 2); + let comparison = Binary::try_new(narrowed.into_array(), constant, op) + .unwrap() + .into_array(); + + let mut ctx = SESSION.create_execution_ctx(); + assert_arrays_eq!( + comparison.optimize().unwrap(), + BoolArray::from_iter([expected, expected]), + &mut ctx + ); + } + #[test] fn test_midnight_binary_comparison() { // DTP with days [0, 1, 2] at midnight From bbe51850206d2c9c429b0b17cfaa895faa25720a Mon Sep 17 00:00:00 2001 From: gitRasheed <99349713+gitRasheed@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:23:45 +0000 Subject: [PATCH 4/4] Shorten comments Signed-off-by: gitRasheed <99349713+gitRasheed@users.noreply.github.com> --- encodings/datetime-parts/src/compress.rs | 14 ++++---------- encodings/datetime-parts/src/compute/compare.rs | 4 +--- encodings/datetime-parts/src/compute/rules.rs | 2 -- encodings/datetime-parts/src/timestamp.rs | 9 ++------- vortex-array/src/aggregate_fn/fns/min_max/mod.rs | 2 -- vortex-array/src/extension/datetime/timestamp.rs | 8 ++------ vortex-btrblocks/src/schemes/temporal.rs | 4 +--- vortex-btrblocks/src/tests.rs | 2 -- 8 files changed, 10 insertions(+), 35 deletions(-) diff --git a/encodings/datetime-parts/src/compress.rs b/encodings/datetime-parts/src/compress.rs index 2d1caecacfb..44406e00226 100644 --- a/encodings/datetime-parts/src/compress.rs +++ b/encodings/datetime-parts/src/compress.rs @@ -22,9 +22,8 @@ use vortex_error::vortex_bail; use crate::timestamp; -/// All parts are stored as i32. Seconds and subseconds always fit in i32. Days are checked by -/// [`split_temporal`]: second and millisecond counts can lie more than `i32::MAX` days from the -/// epoch, and such arrays are rejected rather than wrapped. +/// All parts are stored as i32. Second and millisecond timestamps can lie more than `i32::MAX` +/// days from the epoch, so [`split_temporal`] checks the days. pub struct TemporalParts { pub days: ArrayRef, pub seconds: ArrayRef, @@ -38,8 +37,7 @@ pub struct TemporalParts { /// /// # Errors /// -/// Returns an error if the time unit is days, or if a valid timestamp lies more than `i32::MAX` -/// days from the epoch (see [`can_split_temporal`]). +/// Returns an error if the time unit is days, or if [`can_split_temporal`] is false. pub fn split_temporal(array: TemporalArray, ctx: &mut ExecutionCtx) -> VortexResult { let time_unit = array.temporal_metadata().time_unit(); if matches!(time_unit, TimeUnit::Days) { @@ -124,8 +122,7 @@ pub fn split_temporal(array: TemporalArray, ctx: &mut ExecutionCtx) -> VortexRes }) } -/// Whether `array` can be split into [`TemporalParts`]: every valid timestamp's days since the -/// epoch fit the `i32` days part. Microsecond and nanosecond timestamps always can. +/// Whether every valid timestamp's days since the epoch fit the `i32` days part. pub fn can_split_temporal(array: &TemporalArray, ctx: &mut ExecutionCtx) -> VortexResult { let time_unit = array.temporal_metadata().time_unit(); if matches!(time_unit, TimeUnit::Days) { @@ -134,7 +131,6 @@ pub fn can_split_temporal(array: &TemporalArray, ctx: &mut ExecutionCtx) -> Vort days_fit(array.temporal_values(), time_unit, ctx) } -/// Checks the days of the valid extremes of `values`, which bound every other valid value. fn days_fit(values: &ArrayRef, time_unit: TimeUnit, ctx: &mut ExecutionCtx) -> VortexResult { if matches!(time_unit, TimeUnit::Microseconds | TimeUnit::Nanoseconds) { return Ok(true); @@ -282,8 +278,6 @@ mod tests { )); } - /// Extreme timestamps either split exactly or are rejected, never wrapped: `i64::MAX` seconds - /// used to come back as 50027779063807. #[rstest] #[case::seconds(TimeUnit::Seconds, false)] #[case::milliseconds(TimeUnit::Milliseconds, false)] diff --git a/encodings/datetime-parts/src/compute/compare.rs b/encodings/datetime-parts/src/compute/compare.rs index c6fa15a96a1..9e997db8c10 100644 --- a/encodings/datetime-parts/src/compute/compare.rs +++ b/encodings/datetime-parts/src/compute/compare.rs @@ -52,8 +52,7 @@ impl CompareKernel for DateTimeParts { let Some(options) = ext_dtype.metadata_opt::() else { return Ok(None); }; - // A constant more than i32::MAX days from the epoch has no days part to compare with: - // leave it to the canonical comparison. + // The constant has no i32 days part. let Ok(ts_parts) = timestamp::split(timestamp, options.unit) else { return Ok(None); }; @@ -447,7 +446,6 @@ mod test { ) .unwrap(); - // Timestamp with a value larger than i32::MAX, whose days still fit the i32 days part. let rhs = dtp_array_from_timestamp(i64::from(i32::MAX) + 1, rhs_validity); let comparison = lhs diff --git a/encodings/datetime-parts/src/compute/rules.rs b/encodings/datetime-parts/src/compute/rules.rs index 078c91da45e..e45926ea105 100644 --- a/encodings/datetime-parts/src/compute/rules.rs +++ b/encodings/datetime-parts/src/compute/rules.rs @@ -282,14 +282,12 @@ mod tests { ConstantArray::new(scalar, len).into_array() } - /// A midnight constant whose days do not fit `i32` must not be pushed down as wrapped days. #[test] fn days_pushdown_declines_constant_outside_i32_days() { let constant = midnight_constant(i64::from(i32::MAX) + 1, TimeUnit::Seconds, 1); assert_eq!(try_extract_days_constant(&constant), None); } - /// Days compressed to u16 cannot hold 9999-12-31, so the comparison is not pushed down. #[rstest] #[case::lt(Operator::Lt, true)] #[case::lte(Operator::Lte, true)] diff --git a/encodings/datetime-parts/src/timestamp.rs b/encodings/datetime-parts/src/timestamp.rs index 3395d75ec44..6076bd49f0e 100644 --- a/encodings/datetime-parts/src/timestamp.rs +++ b/encodings/datetime-parts/src/timestamp.rs @@ -30,8 +30,7 @@ pub struct TimestampParts { /// /// # Errors /// -/// Returns an error if `time_unit` is days, which cannot be split, or if the timestamp's days -/// since the epoch do not fit the `i32` days part. +/// Returns an error if `time_unit` is days, or if [`days_fit`] is false. pub fn split(timestamp: i64, time_unit: TimeUnit) -> VortexResult { let parts = match time_unit { TimeUnit::Nanoseconds => split_with_divisor::<1_000_000_000>(timestamp), @@ -50,9 +49,6 @@ pub fn split(timestamp: i64, time_unit: TimeUnit) -> VortexResult bool { let ticks_per_second = match time_unit { TimeUnit::Nanoseconds => 1_000_000_000, @@ -66,8 +62,7 @@ pub fn days_fit(timestamp: i64, time_unit: TimeUnit) -> bool { /// Split a Unix timestamp into parts using a constant DIVISOR. /// -/// Days that do not fit `i32` wrap: callers check [`days_fit`] first, as [`split`] does for a -/// single timestamp and `split_temporal` does for a whole array. +/// Days that do not fit `i32` wrap, so callers check [`days_fit`] first. #[inline] pub(crate) fn split_with_divisor(timestamp: i64) -> TimestampParts { let days = timestamp / (SECONDS_PER_DAY * DIVISOR); diff --git a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs index e04f2b05cbf..707c1aa024b 100644 --- a/vortex-array/src/aggregate_fn/fns/min_max/mod.rs +++ b/vortex-array/src/aggregate_fn/fns/min_max/mod.rs @@ -604,8 +604,6 @@ mod tests { Ok(()) } - /// A timestamp on the last day of year 9999 lies beyond `jiff`'s range but is a valid Arrow - /// and Parquet value, so the statistics of a column holding one must not fail. #[test] fn test_timestamp_min_max_beyond_jiff_range() -> VortexResult<()> { // 9999-12-31T00:00:00Z diff --git a/vortex-array/src/extension/datetime/timestamp.rs b/vortex-array/src/extension/datetime/timestamp.rs index 10bc5eee9c4..5908d3f23d7 100644 --- a/vortex-array/src/extension/datetime/timestamp.rs +++ b/vortex-array/src/extension/datetime/timestamp.rs @@ -109,8 +109,7 @@ impl fmt::Display for TimestampValue<'_> { TimestampValue::Microseconds(v, tz) => (TimeUnit::Microseconds, v, tz), TimestampValue::Nanoseconds(v, tz) => (TimeUnit::Nanoseconds, v, tz), }; - // `jiff` stops 26 hours short of the end of years 9999 and -9999, so a valid timestamp may - // have no calendar rendering; show the count then. + // jiff cannot render the last 26 hours of year 9999, so show the raw count. let Ok(ts) = unit .to_jiff_span(value) .and_then(|span| Ok(jiff::Timestamp::UNIX_EPOCH.checked_add(span)?)) @@ -222,8 +221,7 @@ impl ExtVTable for Timestamp { .map_err(|e| e.with_context("Invalid timezone for timestamp scalar"))?; } - // Every `i64` count of the unit is a valid timestamp, as in Arrow and Parquet. `jiff`'s - // narrower range only bounds what `Display` can render as a calendar date. + // Any i64 count is valid, as in Arrow; jiff's range only limits Display. Ok(match metadata.unit { TimeUnit::Nanoseconds => TimestampValue::Nanoseconds(ts_value, tz), TimeUnit::Microseconds => TimestampValue::Microseconds(ts_value, tz), @@ -257,8 +255,6 @@ mod tests { Ok(()) } - /// Arrow and Parquet store every `i64` count of the unit, so a timestamp on the last day of - /// year 9999, beyond `jiff`'s range, is a valid scalar; it displays as the raw count. #[cfg_attr(miri, ignore)] #[rstest] #[case::end_of_9999( diff --git a/vortex-btrblocks/src/schemes/temporal.rs b/vortex-btrblocks/src/schemes/temporal.rs index 628e42522fe..010f21e0c1d 100644 --- a/vortex-btrblocks/src/schemes/temporal.rs +++ b/vortex-btrblocks/src/schemes/temporal.rs @@ -71,9 +71,7 @@ impl Scheme for TemporalScheme { _compress_ctx: CompressorContext, exec_ctx: &mut ExecutionCtx, ) -> CompressionEstimate { - // Temporal compression (splitting into parts) is almost always beneficial, but second and - // millisecond timestamps more than i32::MAX days from the epoch have no i32 days part, so - // `split_temporal` rejects them and the storage array is compressed instead. + // Second and millisecond timestamps past i32::MAX days cannot be split. let splittable = TemporalArray::try_from(data.array().clone()) .and_then(|temporal| can_split_temporal(&temporal, exec_ctx)); match splittable { diff --git a/vortex-btrblocks/src/tests.rs b/vortex-btrblocks/src/tests.rs index f0843d93cec..384e104008a 100644 --- a/vortex-btrblocks/src/tests.rs +++ b/vortex-btrblocks/src/tests.rs @@ -112,8 +112,6 @@ fn test_default_compressor_roundtrip() -> VortexResult<()> { Ok(()) } -/// Second timestamps beyond i32::MAX days from the epoch have no DateTimeParts form; the -/// compressor must fall back to the storage rather than write wrapped days. #[test] fn test_extreme_second_timestamps_roundtrip() -> VortexResult<()> { let compressor = BtrBlocksCompressorBuilder::from_session(&SESSION)