Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
215 changes: 212 additions & 3 deletions src/commands/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,35 @@ fn value_to_string(v: &Value) -> String {
}
}

/// Warn that a column could not be rendered, once per data type.
///
/// 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::collections::HashSet;
use std::sync::{Mutex, OnceLock};

static WARNED: OnceLock<Mutex<HashSet<arrow::datatypes::DataType>>> = 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!(
"{}",
format!("warning: could not format a {data_type} value, so it is shown as null: {err}")
.yellow()
);
}
Comment thread
anoop-narang marked this conversation as resolved.

/// 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::*;
Expand Down Expand Up @@ -222,9 +251,28 @@ 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)
// `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: <msg>" 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
}
}
}
}
}
Expand Down Expand Up @@ -723,6 +771,167 @@ 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::<i64>]).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"
);
}
}

/// 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: <msg>" 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(
Expand Down
Loading