From deed0936d175f6e6880f634cafe8cbaad9c77953 Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Thu, 3 Sep 2026 14:33:38 +0530 Subject: [PATCH 1/2] fix(query): render named-timezone timestamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Results are fetched as Arrow IPC and formatted client-side, so a timestamp arrives as a raw integer plus a timezone recorded in the schema. Formatting one whose zone is a named IANA zone ("UTC", "America/New_York") needs a timezone database; arrow was built with default features off, so none was compiled in and its formatter returned an error for those values. The cell then reported them as null. Only named zones were affected: a fixed offset ("+00:00") and a zone-less timestamp both format without a timezone database, so a neighbouring column in the same row looked correct and the loss was easy to miss. Nothing was wrong with the data — the same query served inline, formatted by the service, returned the values correctly. Enable arrow's chrono-tz feature, and stop turning a formatting failure into a null: the cell has already been checked for null above, so reaching that arm means a present value could not be rendered, and reporting it as null asserts the opposite. The output shape is kept (callers rely on one value per column) and the failure is reported on stderr instead, once per run so a large result cannot flood it. Two tests cover it: named zones across several time units, and a sweep over the temporal and decimal types a query can return asserting none of them render a present value as null. Both fail without the feature. Regressed in 50e3439, which moved result fetching from JSON to Arrow. --- Cargo.lock | 35 +++++++++ Cargo.toml | 10 ++- src/commands/query.rs | 165 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 206 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c4d436..f485d23 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -159,6 +159,7 @@ dependencies = [ "arrow-data", "arrow-schema", "chrono", + "chrono-tz", "half", "hashbrown 0.15.5", "num", @@ -455,6 +456,16 @@ dependencies = [ "windows-link", ] +[[package]] +name = "chrono-tz" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a6139a8597ed92cf816dfb33f5dd6cf0bb93a6adc938f11039f371bc5bcd26c3" +dependencies = [ + "chrono", + "phf", +] + [[package]] name = "chunked_transfer" version = "1.5.0" @@ -2109,6 +2120,24 @@ version = "2.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" +[[package]] +name = "phf" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "913273894cec178f401a31ec4b656318d95473527be05c0752cc41cdc32be8b7" +dependencies = [ + "phf_shared", +] + +[[package]] +name = "phf_shared" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06005508882fb681fd97892ecff4b7fd0fee13ef1aa569f8695dae7ab9099981" +dependencies = [ + "siphasher", +] + [[package]] name = "pin-project-lite" version = "0.2.17" @@ -2982,6 +3011,12 @@ version = "2.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" +[[package]] +name = "siphasher" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ee5873ec9cce0195efcb7a4e9507a04cd49aec9c83d0389df45b1ef7ba2e649" + [[package]] name = "slab" version = "0.4.12" diff --git a/Cargo.toml b/Cargo.toml index 4afe22a..59184bb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,15 @@ reqwest = { version = "0.13", features = ["blocking", "json"] } rayon = "1.10" serde = { version = "1", features = ["derive"] } serde_json = "1" -arrow = { version = "55", default-features = false, features = ["ipc"] } +arrow = { version = "55", default-features = false, features = [ + "ipc", + # Timestamps carrying a named IANA zone ("UTC", "America/New_York") + # cannot be formatted without a timezone database — arrow's formatter + # returns an error for them, and only fixed offsets ("+00:00") parse + # without it. Results are fetched as Arrow and formatted here, so + # without this feature every named-zone timestamp fails to render. + "chrono-tz", +] } serde_yaml = "0.9" base64 = "0.22" crossterm = "0.28" diff --git a/src/commands/query.rs b/src/commands/query.rs index 69db1c7..534d854 100644 --- a/src/commands/query.rs +++ b/src/commands/query.rs @@ -111,6 +111,24 @@ fn value_to_string(v: &Value) -> String { } } +/// Warn once per process that a column could not be rendered. +/// +/// Deduplicated because the failure is a property of the column's type, not of +/// any one row: a million-row result would otherwise print a million lines. +fn warn_unformattable_once(data_type: &arrow::datatypes::DataType, err: &arrow::error::ArrowError) { + use crossterm::style::Stylize; + use std::sync::atomic::{AtomicBool, Ordering}; + static WARNED: AtomicBool = AtomicBool::new(false); + if WARNED.swap(true, Ordering::Relaxed) { + return; + } + eprintln!( + "{}", + format!("warning: could not format a {data_type} value, so it is shown as null: {err}") + .yellow() + ); +} + /// Convert one cell of an Arrow array to a `serde_json::Value`. fn arrow_cell(col: &dyn arrow::array::Array, row: usize) -> Value { use arrow::array::*; @@ -222,9 +240,20 @@ fn arrow_cell(col: &dyn arrow::array::Array, row: usize) -> Value { _ => { use arrow::util::display::{ArrayFormatter, FormatOptions}; let opts = FormatOptions::default(); - ArrayFormatter::try_new(col, &opts) - .map(|f| Value::String(f.value(row).to_string())) - .unwrap_or(Value::Null) + match ArrayFormatter::try_new(col, &opts) { + Ok(f) => Value::String(f.value(row).to_string()), + // `col.is_null(row)` was checked above, so the cell *has* a + // value and we simply could not render it. Emitting `null` here + // claims the opposite, and is indistinguishable from a real + // null to anything reading the output — which is how a whole + // class of unformattable column went unnoticed. Keep the shape + // (callers rely on a value per column) but say so on stderr, + // once per run so a large result cannot drown the terminal. + Err(e) => { + warn_unformattable_once(col.data_type(), &e); + Value::Null + } + } } } } @@ -723,6 +752,136 @@ mod tests { resp } + /// A timestamp carrying a *named* IANA zone must render, not come back as + /// null. Rendering it needs a timezone database compiled in; without one + /// Arrow's formatter errors, and this cell used to swallow that error and + /// report the value as null — silently, and only for named zones, so a + /// fixed-offset or zone-less timestamp in the same row looked fine. + #[test] + fn named_timezone_timestamps_render_instead_of_nulling() { + use arrow::array::TimestampMicrosecondArray; + + // 2026-01-01T12:00:00Z + let micros = 1_767_268_800_000_000i64; + + for zone in ["UTC", "America/New_York", "Europe/London"] { + let col = TimestampMicrosecondArray::from(vec![micros]).with_timezone(zone); + let cell = arrow_cell(&col, 0); + assert!( + cell.is_string(), + "zone {zone} rendered as {cell:?}, expected a formatted string" + ); + assert_ne!(cell, Value::Null, "zone {zone} rendered as null"); + } + + // The two forms that worked even without a timezone database, kept here + // so a regression narrows to the named-zone case rather than all timestamps. + let offset = TimestampMicrosecondArray::from(vec![micros]).with_timezone("+00:00"); + assert!(arrow_cell(&offset, 0).is_string()); + let naive = TimestampMicrosecondArray::from(vec![micros]); + assert!(arrow_cell(&naive, 0).is_string()); + + // A genuine null is still a null. + let with_null = TimestampMicrosecondArray::from(vec![None::]).with_timezone("UTC"); + assert_eq!(arrow_cell(&with_null, 0), Value::Null); + } + + /// No column type we can be handed may turn a present value into `null`. + /// + /// The timezone case above is one instance of a wider hazard: every type + /// without an explicit arm falls through to Arrow's formatter, and a + /// formatter that cannot handle the type used to be reported as a null + /// cell. This walks the temporal and decimal types a query can return and + /// asserts each renders, so a future dependency or feature change that + /// breaks one of them fails here instead of silently blanking a column. + #[test] + fn no_column_type_silently_renders_as_null() { + use arrow::array::{ + Date32Array, Date64Array, Decimal128Array, DurationMicrosecondArray, Time32SecondArray, + Time64MicrosecondArray, TimestampMicrosecondArray, TimestampMillisecondArray, + TimestampNanosecondArray, TimestampSecondArray, + }; + + let micros = 1_767_268_800_000_000i64; + let cases: Vec<(&str, arrow::array::ArrayRef)> = vec![ + ("Date32", Arc::new(Date32Array::from(vec![20454]))), + ( + "Date64", + Arc::new(Date64Array::from(vec![1_767_268_800_000])), + ), + ("Time32(s)", Arc::new(Time32SecondArray::from(vec![43200]))), + ( + "Time64(µs)", + Arc::new(Time64MicrosecondArray::from(vec![43_200_000_000])), + ), + ( + "Duration(µs)", + Arc::new(DurationMicrosecondArray::from(vec![1_000_000])), + ), + ( + "Decimal128", + Arc::new( + Decimal128Array::from(vec![123_456i128]) + .with_precision_and_scale(10, 3) + .expect("valid decimal"), + ), + ), + ( + "Timestamp(s, naive)", + Arc::new(TimestampSecondArray::from(vec![1_767_268_800])), + ), + ( + "Timestamp(ms, naive)", + Arc::new(TimestampMillisecondArray::from(vec![1_767_268_800_000])), + ), + ( + "Timestamp(µs, naive)", + Arc::new(TimestampMicrosecondArray::from(vec![micros])), + ), + ( + "Timestamp(ns, naive)", + Arc::new(TimestampNanosecondArray::from(vec![micros * 1_000])), + ), + ( + "Timestamp(s, UTC)", + Arc::new(TimestampSecondArray::from(vec![1_767_268_800]).with_timezone("UTC")), + ), + ( + "Timestamp(ms, UTC)", + Arc::new( + TimestampMillisecondArray::from(vec![1_767_268_800_000]).with_timezone("UTC"), + ), + ), + ( + "Timestamp(µs, UTC)", + Arc::new(TimestampMicrosecondArray::from(vec![micros]).with_timezone("UTC")), + ), + ( + "Timestamp(ns, UTC)", + Arc::new(TimestampNanosecondArray::from(vec![micros * 1_000]).with_timezone("UTC")), + ), + ( + "Timestamp(µs, +05:30)", + Arc::new(TimestampMicrosecondArray::from(vec![micros]).with_timezone("+05:30")), + ), + ( + "Timestamp(µs, Asia/Kolkata)", + Arc::new( + TimestampMicrosecondArray::from(vec![micros]).with_timezone("Asia/Kolkata"), + ), + ), + ]; + + for (label, col) in cases { + let cell = arrow_cell(col.as_ref(), 0); + assert_ne!( + cell, + Value::Null, + "{label} has a value at row 0 but rendered as null" + ); + } + } + #[test] fn hint_for_missing_database_context() { let tip = cross_source_hint( From a4b52ec8b3b8bcba40331a3b6683cab8e56a606a Mon Sep 17 00:00:00 2001 From: Anoop Narang Date: Thu, 3 Sep 2026 14:38:42 +0530 Subject: [PATCH 2/2] fix(query): handle per-value format errors, warn per type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review points. The warn-once flag was global, so a result with two unformattable column types reported only the first and the second went back to nulling silently — the exact hazard this removes. Key the set on the data type, which is what the failure is a property of. `try_new` only reports a type Arrow cannot format at all. A single out-of-range value inside a formattable type fails later, in the value formatter, and the plain `Display` impl under the default `safe: true` writes the text "ERROR: " into the cell as if it were data (and panics when the inner write fails). Render the value through `try_to_string` so one bad value degrades the same way a bad type does. Test covers it: an out-of-range Timestamp(Second) must not panic and must not put an error message in the data. Without the change it fails with "ERROR: Cast error: Failed to convert ... to datetime". --- src/commands/query.rs | 80 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/src/commands/query.rs b/src/commands/query.rs index 534d854..31f8db0 100644 --- a/src/commands/query.rs +++ b/src/commands/query.rs @@ -111,15 +111,26 @@ fn value_to_string(v: &Value) -> String { } } -/// Warn once per process that a column could not be rendered. +/// Warn that a column could not be rendered, once per data type. /// -/// Deduplicated because the failure is a property of the column's type, not of -/// any one row: a million-row result would otherwise print a million lines. +/// Keyed on the type rather than a single global flag: the failure is a +/// property of the type, so a million-row result must not print a million +/// lines — but a result with two unformattable types has to report both, or +/// the second one goes back to nulling silently. fn warn_unformattable_once(data_type: &arrow::datatypes::DataType, err: &arrow::error::ArrowError) { use crossterm::style::Stylize; - use std::sync::atomic::{AtomicBool, Ordering}; - static WARNED: AtomicBool = AtomicBool::new(false); - if WARNED.swap(true, Ordering::Relaxed) { + use std::collections::HashSet; + use std::sync::{Mutex, OnceLock}; + + static WARNED: OnceLock>> = OnceLock::new(); + let seen = WARNED.get_or_init(|| Mutex::new(HashSet::new())); + // A poisoned lock only means another thread panicked mid-warn; recover the + // set rather than take the process down over a diagnostic. + let mut seen = match seen.lock() { + Ok(g) => g, + Err(poisoned) => poisoned.into_inner(), + }; + if !seen.insert(data_type.clone()) { return; } eprintln!( @@ -240,15 +251,23 @@ fn arrow_cell(col: &dyn arrow::array::Array, row: usize) -> Value { _ => { use arrow::util::display::{ArrayFormatter, FormatOptions}; let opts = FormatOptions::default(); - match ArrayFormatter::try_new(col, &opts) { - Ok(f) => Value::String(f.value(row).to_string()), - // `col.is_null(row)` was checked above, so the cell *has* a - // value and we simply could not render it. Emitting `null` here - // claims the opposite, and is indistinguishable from a real - // null to anything reading the output — which is how a whole - // class of unformattable column went unnoticed. Keep the shape - // (callers rely on a value per column) but say so on stderr, - // once per run so a large result cannot drown the terminal. + // `col.is_null(row)` was checked above, so the cell *has* a value. + // Reporting it as `null` claims the opposite and is + // indistinguishable from a real null to anything reading the + // output — which is how a whole class of unformattable column went + // unnoticed. Keep the shape (callers rely on a value per column) + // but say so on stderr, deduplicated per type. + // + // Both failure levels are handled. `try_new` reports a type Arrow + // cannot format at all; `try_to_string` reports a single value it + // cannot format. The latter matters because the plain `Display` + // impl, under the default `safe: true`, writes the text + // "ERROR: " into the cell — an error message wearing the + // costume of data — and panics outright when the inner write fails. + let formatted = + ArrayFormatter::try_new(col, &opts).and_then(|f| f.value(row).try_to_string()); + match formatted { + Ok(s) => Value::String(s), Err(e) => { warn_unformattable_once(col.data_type(), &e); Value::Null @@ -882,6 +901,37 @@ mod tests { } } + /// A single value Arrow cannot format must not panic, and must not put an + /// error message into the data. + /// + /// Two distinct failure levels exist: a type Arrow cannot format at all + /// (caught by `try_new`) and one value inside a formattable type that is + /// out of range. For the latter, the plain `Display` impl under the default + /// `safe: true` writes the literal text "ERROR: " as the cell value, + /// and panics when the inner write itself fails. Neither is acceptable in a + /// data column, so the value path is rendered fallibly. + #[test] + fn an_unformattable_value_is_null_not_a_panic_or_an_error_string() { + use arrow::array::TimestampSecondArray; + + // Far outside the range a second-resolution timestamp can render. + let col = TimestampSecondArray::from(vec![i64::MAX]); + let cell = arrow_cell(&col, 0); + + if let Value::String(s) = &cell { + assert!( + !s.contains("ERROR"), + "an error message reached the data as a value: {s}" + ); + } else { + assert_eq!( + cell, + Value::Null, + "expected either a rendered value or null" + ); + } + } + #[test] fn hint_for_missing_database_context() { let tip = cross_source_hint(