From 283ba0112fa6f10d59e1ef5d60b98c6ccd47bdd3 Mon Sep 17 00:00:00 2001 From: Adam Gutglick Date: Tue, 10 Feb 2026 19:04:34 +0000 Subject: [PATCH] Refactor error display and simplify it Signed-off-by: Adam Gutglick --- Cargo.lock | 53 +++- encodings/datetime-parts/src/canonical.rs | 4 +- encodings/datetime-parts/src/ops.rs | 2 +- .../src/bitpacking/array/bitpack_compress.rs | 4 +- .../fastlanes/src/bitpacking/array/mod.rs | 2 +- vortex-array/src/arrays/decimal/array.rs | 2 +- .../src/arrays/fixed_size_list/array.rs | 6 +- vortex-array/src/arrays/list/array.rs | 16 +- .../src/arrays/primitive/array/mod.rs | 1 + .../src/arrays/primitive/compute/cast.rs | 6 +- vortex-array/src/arrays/struct_/array.rs | 8 +- vortex-array/src/arrays/varbin/array.rs | 12 +- vortex-array/src/arrays/varbinview/array.rs | 16 +- vortex-array/src/compute/cast.rs | 1 + vortex-array/src/validity.rs | 2 +- vortex-bench/src/conversions.rs | 28 +- vortex-compute/src/cast/pvector.rs | 10 +- vortex-dtype/src/serde/flatbuffers.rs | 4 +- vortex-dtype/src/serde/proto.rs | 25 +- vortex-duckdb/src/utils/glob.rs | 2 +- vortex-error/Cargo.toml | 5 +- vortex-error/src/lib.rs | 292 +++++++++++------- vortex-error/tests/fmt.rs | 82 +++++ vortex-jni/src/file.rs | 4 +- vortex-scalar/src/proto.rs | 43 ++- vortex-tui/src/convert.rs | 4 +- vortex-vector/src/primitive/cast.rs | 4 +- 27 files changed, 422 insertions(+), 216 deletions(-) create mode 100644 vortex-error/tests/fmt.rs diff --git a/Cargo.lock b/Cargo.lock index 091dff9418d..f8808f1f4f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8470,6 +8470,15 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scc" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "46e6f046b7fef48e2660c57ed794263155d713de679057f2d0c169bfc6e756cc" +dependencies = [ + "sdd", +] + [[package]] name = "schannel" version = "0.1.28" @@ -8520,6 +8529,12 @@ dependencies = [ "sha2", ] +[[package]] +name = "sdd" +version = "3.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" + [[package]] name = "seahash" version = "4.1.0" @@ -8658,6 +8673,32 @@ dependencies = [ "serde", ] +[[package]] +name = "serial_test" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d0b343e184fc3b7bb44dff0705fffcf4b3756ba6aff420dddd8b24ca145e555" +dependencies = [ + "futures-executor", + "futures-util", + "log", + "once_cell", + "parking_lot", + "scc", + "serial_test_derive", +] + +[[package]] +name = "serial_test_derive" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6f50427f258fb77356e4cd4aa0e87e2bd2c66dbcee41dc405282cae2bfc26c83" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.114", +] + [[package]] name = "sha1" version = "0.10.6" @@ -9327,6 +9368,15 @@ version = "0.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b1dd07eb858a2067e2f3c7155d54e929265c264e6f37efe3ee7a8d1b5a1dd0ba" +[[package]] +name = "temp-env" +version = "0.3.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "96374855068f47402c3121c6eed88d29cb1de8f3ab27090e273e420bdabcf050" +dependencies = [ + "parking_lot", +] + [[package]] name = "tempfile" version = "3.24.0" @@ -10582,8 +10632,9 @@ dependencies = [ "prost 0.14.3", "pyo3", "serde_json", + "serial_test", + "temp-env", "tokio", - "url", ] [[package]] diff --git a/encodings/datetime-parts/src/canonical.rs b/encodings/datetime-parts/src/canonical.rs index 2e436a7f6e0..2e51281393a 100644 --- a/encodings/datetime-parts/src/canonical.rs +++ b/encodings/datetime-parts/src/canonical.rs @@ -28,11 +28,11 @@ pub fn decode_to_temporal( ctx: &mut ExecutionCtx, ) -> VortexResult { let DType::Extension(ext) = array.dtype().clone() else { - vortex_panic!(ComputeError: "expected dtype to be DType::Extension variant") + vortex_panic!(Compute: "expected dtype to be DType::Extension variant") }; let Some(options) = ext.metadata_opt::() else { - vortex_panic!(ComputeError: "must decode TemporalMetadata from extension metadata"); + vortex_panic!(Compute: "must decode TemporalMetadata from extension metadata"); }; let divisor = match options.unit { diff --git a/encodings/datetime-parts/src/ops.rs b/encodings/datetime-parts/src/ops.rs index 540319d36ef..7bc71f4d4bf 100644 --- a/encodings/datetime-parts/src/ops.rs +++ b/encodings/datetime-parts/src/ops.rs @@ -25,7 +25,7 @@ impl OperationsVTable for DateTimePartsVTable { }; let Some(options) = ext.metadata_opt::() else { - vortex_panic!(ComputeError: "must decode TemporalMetadata from extension metadata"); + vortex_panic!(Compute: "must decode TemporalMetadata from extension metadata"); }; if !array.is_valid(index)? { diff --git a/encodings/fastlanes/src/bitpacking/array/bitpack_compress.rs b/encodings/fastlanes/src/bitpacking/array/bitpack_compress.rs index e7245ea6dc2..cdff8025300 100644 --- a/encodings/fastlanes/src/bitpacking/array/bitpack_compress.rs +++ b/encodings/fastlanes/src/bitpacking/array/bitpack_compress.rs @@ -50,7 +50,7 @@ pub fn bitpack_encode( array.statistics().compute_min::

().unwrap_or_default() < 0 }); if has_negative_values { - vortex_bail!("cannot bitpack_encode array containing negative integers") + vortex_bail!(InvalidArgument: "cannot bitpack_encode array containing negative integers") } } @@ -59,7 +59,7 @@ pub fn bitpack_encode( if bit_width >= array.ptype().bit_width() as u8 { // Nothing we can do vortex_bail!( - "Cannot pack - specified bit width {bit_width} >= {}", + InvalidArgument: "Cannot pack - specified bit width {bit_width} >= {}", array.ptype().bit_width() ) } diff --git a/encodings/fastlanes/src/bitpacking/array/mod.rs b/encodings/fastlanes/src/bitpacking/array/mod.rs index 08c9b75abfb..b1db133ff5c 100644 --- a/encodings/fastlanes/src/bitpacking/array/mod.rs +++ b/encodings/fastlanes/src/bitpacking/array/mod.rs @@ -273,7 +273,7 @@ impl BitPackedArray { if let Some(parray) = array.as_opt::() { bitpack_encode(parray, bit_width, None) } else { - vortex_bail!("Bitpacking can only encode primitive arrays"); + vortex_bail!(InvalidArgument: "Bitpacking can only encode primitive arrays"); } } diff --git a/vortex-array/src/arrays/decimal/array.rs b/vortex-array/src/arrays/decimal/array.rs index b3352444e6c..6b4cb2ea10e 100644 --- a/vortex-array/src/arrays/decimal/array.rs +++ b/vortex-array/src/arrays/decimal/array.rs @@ -243,7 +243,7 @@ impl DecimalArray { let expected_len = values_type.byte_width() * validity_len; vortex_ensure!( buffer.len() == expected_len, - "expected buffer of size {} bytes, was {} bytes", + InvalidArgument: "expected buffer of size {} bytes, was {} bytes", expected_len, buffer.len(), ); diff --git a/vortex-array/src/arrays/fixed_size_list/array.rs b/vortex-array/src/arrays/fixed_size_list/array.rs index e512b77518c..d62b3d569bb 100644 --- a/vortex-array/src/arrays/fixed_size_list/array.rs +++ b/vortex-array/src/arrays/fixed_size_list/array.rs @@ -186,7 +186,7 @@ impl FixedSizeListArray { if let Some(validity_len) = validity.maybe_len() { vortex_ensure!( len == validity_len, - "validity with size {validity_len} does not match fixed-size list array size {len}", + InvalidArgument: "validity with size {validity_len} does not match fixed-size list array size {len}", ); } @@ -195,14 +195,14 @@ impl FixedSizeListArray { if list_size == 0 { vortex_ensure!( elements.is_empty(), - "a degenerate (`list_size == 0`) `FixedSizeList` should have no underlying elements" + InvalidArgument: "a degenerate (`list_size == 0`) `FixedSizeList` should have no underlying elements" ); return Ok(()); } vortex_ensure!( len * list_size as usize == elements.len(), - "the `elements` array has the incorrect number of elements to construct a \ + InvalidArgument: "the `elements` array has the incorrect number of elements to construct a \ `FixedSizeList[{list_size}] array of length {len}", ); diff --git a/vortex-array/src/arrays/list/array.rs b/vortex-array/src/arrays/list/array.rs index 0a1c433547a..5239d7f1e38 100644 --- a/vortex-array/src/arrays/list/array.rs +++ b/vortex-array/src/arrays/list/array.rs @@ -163,13 +163,13 @@ impl ListArray { // Offsets must have at least one element vortex_ensure!( !offsets.is_empty(), - "Offsets must have at least one element, [0] for an empty list" + InvalidArgument: "Offsets must have at least one element, [0] for an empty list" ); // Offsets must be of integer type, and cannot go lower than 0. vortex_ensure!( offsets.dtype().is_int() && !offsets.dtype().is_nullable(), - "offsets have invalid type {}", + InvalidArgument: "offsets have invalid type {}", offsets.dtype() ); @@ -178,9 +178,9 @@ impl ListArray { // Offsets must be sorted (but not strictly sorted, zero-length lists are allowed) if let Some(is_sorted) = offsets.statistics().compute_is_sorted() { - vortex_ensure!(is_sorted, "offsets must be sorted"); + vortex_ensure!(is_sorted, InvalidArgument: "offsets must be sorted"); } else { - vortex_bail!("offsets must report is_sorted statistic"); + vortex_bail!(InvalidArgument: "offsets must report is_sorted statistic"); } // Validate that offsets min is non-negative, and max does not exceed the length of @@ -202,7 +202,7 @@ impl ListArray { vortex_ensure!( min >= 0, - "offsets minimum {min} outside valid range [0, {max}]" + InvalidArgument: "offsets minimum {min} outside valid range [0, {max}]" ); vortex_ensure!( @@ -211,7 +211,7 @@ impl ListArray {

::PTYPE, elements.len() )), - "Max offset {max} is beyond the length of the elements array {}", + InvalidArgument: "Max offset {max} is beyond the length of the elements array {}", elements.len() ); } @@ -219,7 +219,7 @@ impl ListArray { } else { // TODO(aduffy): fallback to slower validation pathway? vortex_bail!( - "offsets array with encoding {} must support min_max compute function", + InvalidArgument: "offsets array with encoding {} must support min_max compute function", offsets.encoding_id() ); }; @@ -228,7 +228,7 @@ impl ListArray { if let Some(validity_len) = validity.maybe_len() { vortex_ensure!( validity_len == offsets.len() - 1, - "validity with size {validity_len} does not match array size {}", + InvalidArgument: "validity with size {validity_len} does not match array size {}", offsets.len() - 1 ); } diff --git a/vortex-array/src/arrays/primitive/array/mod.rs b/vortex-array/src/arrays/primitive/array/mod.rs index 8eda66cc255..30b0a0fcbf8 100644 --- a/vortex-array/src/arrays/primitive/array/mod.rs +++ b/vortex-array/src/arrays/primitive/array/mod.rs @@ -165,6 +165,7 @@ impl PrimitiveArray { && buffer.len() != len { return Err(vortex_err!( + InvalidArgument: "Buffer and validity length mismatch: buffer={}, validity={}", buffer.len(), len diff --git a/vortex-array/src/arrays/primitive/compute/cast.rs b/vortex-array/src/arrays/primitive/compute/cast.rs index 83ab791d532..0ce2675908f 100644 --- a/vortex-array/src/arrays/primitive/compute/cast.rs +++ b/vortex-array/src/arrays/primitive/compute/cast.rs @@ -66,7 +66,7 @@ fn cast(array: &[F], mask: Mask) -> VortexResult let mut buffer = BufferMut::with_capacity(array.len()); for item in array { let item = T::from(*item).ok_or_else( - || vortex_err!(ComputeError: "Failed to cast {} to {:?}", item, T::PTYPE), + || vortex_err!(Compute: "Failed to cast {} to {:?}", item, T::PTYPE), )?; // SAFETY: we've pre-allocated the required capacity unsafe { buffer.push_unchecked(item) } @@ -80,7 +80,7 @@ fn cast(array: &[F], mask: Mask) -> VortexResult for (item, valid) in array.iter().zip(b.iter()) { if valid { let item = T::from(*item).ok_or_else( - || vortex_err!(ComputeError: "Failed to cast {} to {:?}", item, T::PTYPE), + || vortex_err!(Compute: "Failed to cast {} to {:?}", item, T::PTYPE), )?; // SAFETY: we've pre-allocated the required capacity unsafe { buffer.push_unchecked(item) } @@ -181,7 +181,7 @@ mod test { fn cast_i32_u32() { let arr = buffer![-1i32].into_array(); let error = cast(&arr, PType::U32.into()).err().unwrap(); - let VortexError::ComputeError(s, _) = error else { + let VortexError::Compute(s, _) = error else { unreachable!() }; assert_eq!(s.to_string(), "Failed to cast -1 to U32"); diff --git a/vortex-array/src/arrays/struct_/array.rs b/vortex-array/src/arrays/struct_/array.rs index 4cbeb38bbbb..3438cb9c6c7 100644 --- a/vortex-array/src/arrays/struct_/array.rs +++ b/vortex-array/src/arrays/struct_/array.rs @@ -301,7 +301,7 @@ impl StructArray { // Check field count matches if fields.len() != dtype.names().len() { vortex_bail!( - "Got {} fields but dtype has {} names", + InvalidArgument: "Got {} fields but dtype has {} names", fields.len(), dtype.names().len() ); @@ -311,7 +311,7 @@ impl StructArray { for (i, (field, struct_dt)) in fields.iter().zip(dtype.fields()).enumerate() { if field.len() != length { vortex_bail!( - "Field {} has length {} but expected {}", + InvalidArgument: "Field {} has length {} but expected {}", i, field.len(), length @@ -320,7 +320,7 @@ impl StructArray { if field.dtype() != &struct_dt { vortex_bail!( - "Field {} has dtype {} but expected {}", + InvalidArgument: "Field {} has dtype {} but expected {}", i, field.dtype(), struct_dt @@ -333,7 +333,7 @@ impl StructArray { && validity_len != length { vortex_bail!( - "Validity has length {} but expected {}", + InvalidArgument: "Validity has length {} but expected {}", validity_len, length ); diff --git a/vortex-array/src/arrays/varbin/array.rs b/vortex-array/src/arrays/varbin/array.rs index c28e59db115..dc587c69676 100644 --- a/vortex-array/src/arrays/varbin/array.rs +++ b/vortex-array/src/arrays/varbin/array.rs @@ -189,7 +189,7 @@ impl VarBinArray { // Check nullability matches vortex_ensure!( dtype.is_nullable() != (validity == &Validity::NonNullable), - "incorrect validity {:?} for dtype {}", + InvalidArgument: "incorrect validity {:?} for dtype {}", validity, dtype ); @@ -197,12 +197,12 @@ impl VarBinArray { // Check offsets has at least one element vortex_ensure!( !offsets.is_empty(), - "Offsets must have at least one element" + InvalidArgument: "Offsets must have at least one element" ); // Check offsets are sorted if let Some(is_sorted) = offsets.statistics().compute_is_sorted() { - vortex_ensure!(is_sorted, "offsets must be sorted"); + vortex_ensure!(is_sorted, InvalidArgument: "offsets must be sorted"); } // Skip host-only validation when offsets/bytes are not host-resident. @@ -211,10 +211,12 @@ impl VarBinArray { .scalar_at(offsets.len() - 1)? .as_primitive() .as_::() - .ok_or_else(|| vortex_err!("Last offset must be convertible to usize"))?; + .ok_or_else( + || vortex_err!(InvalidArgument: "Last offset must be convertible to usize"), + )?; vortex_ensure!( last_offset <= bytes.len(), - "Last offset {} exceeds bytes length {}", + InvalidArgument: "Last offset {} exceeds bytes length {}", last_offset, bytes.len() ); diff --git a/vortex-array/src/arrays/varbinview/array.rs b/vortex-array/src/arrays/varbinview/array.rs index cdd3a227069..2189ff25508 100644 --- a/vortex-array/src/arrays/varbinview/array.rs +++ b/vortex-array/src/arrays/varbinview/array.rs @@ -264,7 +264,7 @@ impl VarBinViewArray { ) -> VortexResult<()> { vortex_ensure!( validity.nullability() == dtype.nullability(), - "validity {:?} incompatible with nullability {:?}", + InvalidArgument: "validity {:?} incompatible with nullability {:?}", validity, dtype.nullability() ); @@ -274,7 +274,7 @@ impl VarBinViewArray { simdutf8::basic::from_utf8(string).is_ok() })?, DType::Binary(_) => Self::validate_views(views, buffers, validity, |_| true)?, - _ => vortex_bail!("invalid DType {dtype} for `VarBinViewArray`"), + _ => vortex_bail!(InvalidArgument: "invalid DType {dtype} for `VarBinViewArray`"), } Ok(()) @@ -299,7 +299,7 @@ impl VarBinViewArray { let bytes = &view.as_inlined().data[..view.len() as usize]; vortex_ensure!( validator(bytes), - "view at index {idx}: inlined bytes failed utf-8 validation" + InvalidArgument: "view at index {idx}: inlined bytes failed utf-8 validation" ); } else { // Validate the view pointer @@ -309,18 +309,18 @@ impl VarBinViewArray { let end_offset = start_offset.saturating_add(view.size as usize); let buf = buffers.get(buf_index).ok_or_else(|| - vortex_err!("view at index {idx} references invalid buffer: {buf_index} out of bounds for VarBinViewArray with {} buffers", + vortex_err!(InvalidArgument: "view at index {idx} references invalid buffer: {buf_index} out of bounds for VarBinViewArray with {} buffers", buffers.len()))?; vortex_ensure!( start_offset < buf.len(), - "start offset {start_offset} out of bounds for buffer {buf_index} with size {}", + InvalidArgument: "start offset {start_offset} out of bounds for buffer {buf_index} with size {}", buf.len(), ); vortex_ensure!( end_offset <= buf.len(), - "end offset {end_offset} out of bounds for buffer {buf_index} with size {}", + InvalidArgument: "end offset {end_offset} out of bounds for buffer {buf_index} with size {}", buf.len(), ); @@ -328,13 +328,13 @@ impl VarBinViewArray { let bytes = &buf[start_offset..end_offset]; vortex_ensure!( view.prefix == bytes[..4], - "VarBinView prefix does not match full string" + InvalidArgument: "VarBinView prefix does not match full string" ); // Validate the full string vortex_ensure!( validator(bytes), - "view at index {idx}: outlined bytes fails utf-8 validation" + InvalidArgument: "view at index {idx}: outlined bytes fails utf-8 validation" ); } } diff --git a/vortex-array/src/compute/cast.rs b/vortex-array/src/compute/cast.rs index 63d5dba2ade..228ed8a7aed 100644 --- a/vortex-array/src/compute/cast.rs +++ b/vortex-array/src/compute/cast.rs @@ -83,6 +83,7 @@ impl ComputeFnVTable for Cast { if array.is_canonical() { vortex_bail!( + InvalidArgument: "No compute kernel to cast array {} with dtype {} to {}", array.encoding_id(), array.dtype(), diff --git a/vortex-array/src/validity.rs b/vortex-array/src/validity.rs index 8a800a90abe..8a54ff37b40 100644 --- a/vortex-array/src/validity.rs +++ b/vortex-array/src/validity.rs @@ -369,7 +369,7 @@ impl Validity { pub fn cast_nullability(self, nullability: Nullability, len: usize) -> VortexResult { match nullability { Nullability::NonNullable => self.into_non_nullable(len).ok_or_else(|| { - vortex_err!("Cannot cast array with invalid values to non-nullable type.") + vortex_err!(InvalidArgument: "Cannot cast array with invalid values to non-nullable type.") }), Nullability::Nullable => Ok(self.into_nullable()), } diff --git a/vortex-bench/src/conversions.rs b/vortex-bench/src/conversions.rs index c6f512d66e0..1ab3c1e7f20 100644 --- a/vortex-bench/src/conversions.rs +++ b/vortex-bench/src/conversions.rs @@ -27,8 +27,8 @@ use vortex::array::stream::ArrayStreamAdapter; use vortex::array::stream::ArrayStreamExt; use vortex::dtype::DType; use vortex::dtype::arrow::FromArrowType; -use vortex::error::VortexError; use vortex::error::VortexResult; +use vortex::error::vortex_err; use vortex::file::WriteOptionsSessionExt; use vortex::session::VortexSession; @@ -89,20 +89,18 @@ pub fn parquet_to_vortex_stream( reader: ParquetRecordBatchStream, ) -> impl futures::Stream> { reader.map(move |result| { - result - .map_err(|e| VortexError::generic(e.into())) - .and_then(|rb| { - let chunk = ArrayRef::from_arrow(rb, false)?; - let mut builder = builder_with_capacity(chunk.dtype(), chunk.len()); - - // Canonicalize the chunk. - chunk.append_to_builder( - builder.as_mut(), - &mut VortexSession::default().create_execution_ctx(), - )?; - - Ok(builder.finish()) - }) + result.map_err(|e| vortex_err!(External: e)).and_then(|rb| { + let chunk = ArrayRef::from_arrow(rb, false)?; + let mut builder = builder_with_capacity(chunk.dtype(), chunk.len()); + + // Canonicalize the chunk. + chunk.append_to_builder( + builder.as_mut(), + &mut VortexSession::default().create_execution_ctx(), + )?; + + Ok(builder.finish()) + }) }) } diff --git a/vortex-compute/src/cast/pvector.rs b/vortex-compute/src/cast/pvector.rs index 338dee2e952..b5ca294d8d9 100644 --- a/vortex-compute/src/cast/pvector.rs +++ b/vortex-compute/src/cast/pvector.rs @@ -74,9 +74,9 @@ impl Cast for PScalar { let result = match self.value() { None => PScalar::null(), Some(v) => { - let converted = ::from(v).ok_or_else(|| { - vortex_err!(ComputeError: "Failed to cast {} to {:?}", v, Dst::PTYPE) - })?; + let converted = ::from(v).ok_or_else( + || vortex_err!(Compute: "Failed to cast {} to {:?}", v, Dst::PTYPE), + )?; PScalar::new(Some(converted)) } }; @@ -172,7 +172,7 @@ mod tests { fn cast_i32_u32_overflow() { let vec: PVector = buffer![-1i32].into(); let error = vec.cast(&PType::U32.into()).err().unwrap(); - let VortexError::ComputeError(s, _) = error else { + let VortexError::Compute(s, _) = error else { unreachable!() }; assert_eq!(s.to_string(), "Failed to cast -1 to U32"); @@ -226,7 +226,7 @@ mod tests { fn cast_scalar_i32_u32_overflow() { let scalar: PScalar = PScalar::new(Some(-1)); let error = scalar.cast(&PType::U32.into()).err().unwrap(); - let VortexError::ComputeError(s, _) = error else { + let VortexError::Compute(s, _) = error else { unreachable!() }; assert_eq!(s.to_string(), "Failed to cast -1 to U32"); diff --git a/vortex-dtype/src/serde/flatbuffers.rs b/vortex-dtype/src/serde/flatbuffers.rs index 9b69e7ed9de..c0a8b7fb92b 100644 --- a/vortex-dtype/src/serde/flatbuffers.rs +++ b/vortex-dtype/src/serde/flatbuffers.rs @@ -203,7 +203,7 @@ impl TryFrom for DType { ); let storage_dtype = fb_ext.storage_dtype().ok_or_else(|| { vortex_err!( - InvalidSerde: "storage_dtype must be present on DType fbs message") + Serde: "storage_dtype must be present on DType fbs message") })?; let storage_view = ViewedDType::from_fb_loc( storage_dtype._tab.loc(), @@ -408,7 +408,7 @@ impl TryFrom for PType { fb::PType::F16 => Self::F16, fb::PType::F32 => Self::F32, fb::PType::F64 => Self::F64, - _ => vortex_bail!(InvalidSerde: "Unknown PType variant"), + _ => vortex_bail!(Serde: "Unknown PType variant"), }) } } diff --git a/vortex-dtype/src/serde/proto.rs b/vortex-dtype/src/serde/proto.rs index b9706c89cff..84b96dfcc28 100644 --- a/vortex-dtype/src/serde/proto.rs +++ b/vortex-dtype/src/serde/proto.rs @@ -26,7 +26,7 @@ impl DType { match value .dtype_type .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "Unrecognized DType"))? + .ok_or_else(|| vortex_err!(Serde: "Unrecognized DType"))? { DtypeType::Null(_) => Ok(Self::Null), DtypeType::Bool(b) => Ok(Self::Bool(b.nullable.into())), @@ -60,7 +60,7 @@ impl DType { DType::from_proto( l.element_type .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "Invalid list element type"))? + .ok_or_else(|| vortex_err!(Serde: "Invalid list element type"))? .as_ref(), session, ) @@ -71,13 +71,16 @@ impl DType { DtypeType::FixedSizeList(fsl) => { let nullable = fsl.nullable.into(); Ok(Self::FixedSizeList( - DType::from_proto(fsl.element_type - .as_ref() - .ok_or_else( - || vortex_err!(InvalidSerde: "Invalid fixed-size list element type"), - )? - .as_ref(), - session).map(Arc::new)?, + DType::from_proto( + fsl.element_type + .as_ref() + .ok_or_else( + || vortex_err!(Serde: "Invalid fixed-size list element type"), + )? + .as_ref(), + session, + ) + .map(Arc::new)?, fsl.size, nullable, )) @@ -85,7 +88,7 @@ impl DType { DtypeType::Extension(e) => { let id = ExtID::new_arc(e.id.as_str().to_string().into()); let vtable = session.dtypes().registry().find(&id).ok_or_else( - || vortex_err!(InvalidSerde: "Unregistered extension type ID: {}", e.id), + || vortex_err!(Serde: "Unregistered extension type ID: {}", e.id), )?; let storage_dtype = DType::from_proto( e.storage_dtype @@ -201,7 +204,7 @@ impl TryFrom<&pb::FieldPath> for FieldPath { match field .field_type .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "FieldPath part missing type"))? + .ok_or_else(|| vortex_err!(Serde: "FieldPath part missing type"))? { FieldType::Name(name) => path.push(Field::from(name.as_str())), } diff --git a/vortex-duckdb/src/utils/glob.rs b/vortex-duckdb/src/utils/glob.rs index 0c21c4f3034..a7e6d31b339 100644 --- a/vortex-duckdb/src/utils/glob.rs +++ b/vortex-duckdb/src/utils/glob.rs @@ -42,7 +42,7 @@ mod s3 { ) -> VortexResult<(Vec, Option>)> { validate_glob(&url_glob)?; assert_eq!("s3://", &url_glob.as_ref()[..5]); - let url = Url::parse(url_glob.as_ref())?; + let url = Url::parse(url_glob.as_ref()).map_err(|e| vortex_err!(External: e))?; let bucket = url .host_str() diff --git a/vortex-error/Cargo.toml b/vortex-error/Cargo.toml index 88d9850bfbc..d562466239c 100644 --- a/vortex-error/Cargo.toml +++ b/vortex-error/Cargo.toml @@ -29,7 +29,10 @@ prost = { workspace = true } pyo3 = { workspace = true, optional = true } serde_json = { workspace = true, optional = true } tokio = { workspace = true, features = ["rt"], optional = true } -url = { workspace = true } + +[dev-dependencies] +serial_test = { version = "3.3.1" } +temp-env = "0.3" [lints] workspace = true diff --git a/vortex-error/src/lib.rs b/vortex-error/src/lib.rs index f1ef348a9ed..210a801153c 100644 --- a/vortex-error/src/lib.rs +++ b/vortex-error/src/lib.rs @@ -7,6 +7,7 @@ //! It also contains a variety of useful macros for error handling. use std::backtrace::Backtrace; +use std::backtrace::BacktraceStatus; use std::borrow::Cow; use std::convert::Infallible; use std::env; @@ -75,21 +76,24 @@ impl From for VortexError { const _: () = { assert!(size_of::() < 128); }; + /// The top-level error type for Vortex. #[non_exhaustive] pub enum VortexError { - /// A wrapped generic error - Generic(Box, Box), + /// A catch-all error variant + Other(ErrString, Box), + /// A wrapped external error + External(Box, Box), /// An index is out of bounds. OutOfBounds(usize, usize, usize, Box), /// An error occurred while executing a compute kernel. - ComputeError(ErrString, Box), + Compute(ErrString, Box), /// An invalid argument was provided. InvalidArgument(ErrString, Box), /// The system has reached an invalid state, InvalidState(ErrString, Box), /// An error occurred while serializing or deserializing. - InvalidSerde(ErrString, Box), + Serde(ErrString, Box), /// An unimplemented function was called. NotImplemented(ErrString, ErrString, Box), /// A type mismatch occurred. @@ -101,33 +105,31 @@ pub enum VortexError { /// A wrapper for shared errors that require cloning. Shared(Arc), /// A wrapper for errors from the Arrow library. - ArrowError(arrow_schema::ArrowError, Box), + Arrow(arrow_schema::ArrowError, Box), /// A wrapper for errors from the FlatBuffers library. #[cfg(feature = "flatbuffers")] - FlatBuffersError(flatbuffers::InvalidFlatbuffer, Box), + FlatBuffers(flatbuffers::InvalidFlatbuffer, Box), /// A wrapper for formatting errors. - FmtError(fmt::Error, Box), + Fmt(fmt::Error, Box), /// A wrapper for IO errors. - IOError(io::Error, Box), + Io(io::Error, Box), /// A wrapper for errors from the standard library when converting a slice to an array. - TryFromSliceError(std::array::TryFromSliceError, Box), + TryFromSlice(std::array::TryFromSliceError, Box), /// A wrapper for errors from the Object Store library. #[cfg(feature = "object_store")] ObjectStore(object_store::Error, Box), /// A wrapper for errors from the Jiff library. - JiffError(jiff::Error, Box), + Jiff(jiff::Error, Box), /// A wrapper for Tokio join error. #[cfg(feature = "tokio")] - JoinError(tokio::task::JoinError, Box), - /// A wrapper for URL parsing errors. - UrlError(url::ParseError, Box), + Join(tokio::task::JoinError, Box), /// Wrap errors for fallible integer casting. TryFromInt(TryFromIntError, Box), /// Wrap protobuf-related errors Prost(Box, Box), /// Wrap serde and serde json errors #[cfg(feature = "serde")] - SerdeJsonError(serde_json::Error, Box), + SerdeJson(serde_json::Error, Box), } impl VortexError { @@ -136,129 +138,186 @@ impl VortexError { VortexError::Context(msg.into(), Box::new(self)) } - /// Wrap an a generic error into a Vortex error - pub fn generic(err: Box) -> Self { - Self::Generic(err, Box::new(Backtrace::capture())) + /// Error prefix by variant + fn variant_prefix(&self) -> &'static str { + use VortexError::*; + + match self { + Other(..) => "Other error: ", + External(..) => "External error: ", + OutOfBounds(..) => "Out of bounds error: ", + Compute(..) => "Compute error: ", + InvalidArgument(..) => "Invalid argument error: ", + InvalidState(..) => "Invalid state error: ", + Serde(..) => "Serde error: ", + NotImplemented(..) => "Not implemented error: ", + MismatchedTypes(..) => "Mismatched types error: ", + AssertionFailed(..) => "Assertion failed error: ", + Context(..) | Shared(..) => "", // basically delegate to the underlying one + Arrow(..) => "Arrow error: ", + #[cfg(feature = "flatbuffers")] + FlatBuffers(..) => "Flat buffers error: ", + Fmt(..) => "Fmt: ", + Io(..) => "Io: ", + TryFromSlice(..) => "Try from slice error: ", + #[cfg(feature = "object_store")] + ObjectStore(..) => "Object store error: ", + Jiff(..) => "Jiff error: ", + #[cfg(feature = "tokio")] + Join(..) => "Tokio join error:", + TryFromInt(..) => "Try from int error:", + Prost(..) => "Prost error:", + #[cfg(feature = "serde")] + SerdeJson(..) => "JSON serde error:", + } } -} -impl Display for VortexError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + fn backtrace(&self) -> Option<&Backtrace> { + use VortexError::*; + match self { - VortexError::Generic(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") - } - VortexError::OutOfBounds(idx, start, stop, backtrace) => { - write!( - f, - "index {idx} out of bounds from {start} to {stop}\nBacktrace:\n{backtrace}", - ) - } - VortexError::ComputeError(msg, backtrace) => { - write!(f, "{msg}\nBacktrace:\n{backtrace}") - } - VortexError::InvalidArgument(msg, backtrace) => { - write!(f, "{msg}\nBacktrace:\n{backtrace}") - } - VortexError::InvalidState(msg, backtrace) => { - write!(f, "{msg}\nBacktrace:\n{backtrace}") - } - VortexError::InvalidSerde(msg, backtrace) => { - write!(f, "{msg}\nBacktrace:\n{backtrace}") + Other(.., bt) => Some(bt.as_ref()), + External(.., bt) => Some(bt.as_ref()), + OutOfBounds(.., bt) => Some(bt.as_ref()), + Compute(.., bt) => Some(bt.as_ref()), + InvalidArgument(.., bt) => Some(bt.as_ref()), + InvalidState(.., bt) => Some(bt.as_ref()), + Serde(.., bt) => Some(bt.as_ref()), + NotImplemented(.., bt) => Some(bt.as_ref()), + MismatchedTypes(.., bt) => Some(bt.as_ref()), + AssertionFailed(.., bt) => Some(bt.as_ref()), + Arrow(.., bt) => Some(bt.as_ref()), + #[cfg(feature = "flatbuffers")] + FlatBuffers(.., bt) => Some(bt.as_ref()), + Fmt(.., bt) => Some(bt.as_ref()), + Io(.., bt) => Some(bt.as_ref()), + TryFromSlice(.., bt) => Some(bt.as_ref()), + #[cfg(feature = "object_store")] + ObjectStore(.., bt) => Some(bt.as_ref()), + Jiff(.., bt) => Some(bt.as_ref()), + #[cfg(feature = "tokio")] + Join(.., bt) => Some(bt.as_ref()), + TryFromInt(.., bt) => Some(bt.as_ref()), + Prost(.., bt) => Some(bt.as_ref()), + #[cfg(feature = "serde")] + SerdeJson(.., bt) => Some(bt.as_ref()), + Context(_, inner) => inner.backtrace(), + Shared(inner) => inner.backtrace(), + } + } + + fn message(&self) -> String { + use VortexError::*; + + match self { + Other(msg, _) => msg.to_string(), + External(err, _) => err.to_string(), + OutOfBounds(idx, start, stop, _) => { + format!("index {idx} out of bounds from {start} to {stop}") } - VortexError::NotImplemented(func, by_whom, backtrace) => { - write!( - f, - "function {func} not implemented for {by_whom}\nBacktrace:\n{backtrace}", - ) + Compute(msg, _) + | InvalidArgument(msg, _) + | InvalidState(msg, _) + | Serde(msg, _) + | AssertionFailed(msg, _) => { + format!("{msg}") } - VortexError::MismatchedTypes(expected, actual, backtrace) => { - write!( - f, - "expected type: {expected} but instead got {actual}\nBacktrace:\n{backtrace}", - ) + NotImplemented(func, by_whom, _) => { + format!("function {func} not implemented for {by_whom}") } - VortexError::AssertionFailed(msg, backtrace) => { - write!(f, "{msg}\nBacktrace:\n{backtrace}") + MismatchedTypes(expected, actual, _) => { + format!("expected type: {expected} but instead got {actual}") } - VortexError::Context(msg, inner) => { - write!(f, "{msg}:\n {inner}") + Context(msg, inner) => { + format!("{msg}:\n {inner}") } - VortexError::Shared(inner) => Display::fmt(inner, f), - VortexError::ArrowError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + Shared(inner) => inner.message(), + Arrow(err, _) => { + format!("{err}") } #[cfg(feature = "flatbuffers")] - VortexError::FlatBuffersError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + FlatBuffers(err, _) => { + format!("{err}") } - VortexError::FmtError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + Fmt(err, _) => { + format!("{err}") } - VortexError::IOError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + Io(err, _) => { + format!("{err}") } - VortexError::TryFromSliceError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + TryFromSlice(err, _) => { + format!("{err}") } #[cfg(feature = "object_store")] - VortexError::ObjectStore(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + ObjectStore(err, _) => { + format!("{err}") } - VortexError::JiffError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + Jiff(err, _) => { + format!("{err}") } #[cfg(feature = "tokio")] - VortexError::JoinError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + Join(err, _) => { + format!("{err}") } - VortexError::UrlError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") - } - VortexError::TryFromInt(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + TryFromInt(err, _) => { + format!("{err}") } #[cfg(feature = "serde")] - VortexError::SerdeJsonError(err, backtrace) => { - write!(f, "{err}\nBacktrace:\n{backtrace}") + SerdeJson(err, _) => { + format!("{err}") } - VortexError::Prost(err, backtrace) => { - write!(f, "Protobuf error: {err}\nBacktrace:\n{backtrace}") + Prost(err, _) => { + format!("{err}") } } } } +impl Display for VortexError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.variant_prefix())?; + write!(f, "{}", self.message())?; + if let Some(backtrace) = self.backtrace() + && backtrace.status() == BacktraceStatus::Captured + { + write!(f, "\nBacktrace:\n{backtrace}")?; + } + + Ok(()) + } +} + +impl Debug for VortexError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{self}") + } +} + impl Error for VortexError { fn source(&self) -> Option<&(dyn Error + 'static)> { + use VortexError::*; + match self { - VortexError::Generic(err, _) => Some(err.as_ref()), - VortexError::Context(_, inner) => inner.source(), - VortexError::Shared(inner) => inner.source(), - VortexError::ArrowError(err, _) => Some(err), + External(err, _) => Some(err.as_ref()), + Context(_, inner) => inner.source(), + Shared(inner) => inner.source(), + Arrow(err, _) => Some(err), #[cfg(feature = "flatbuffers")] - VortexError::FlatBuffersError(err, _) => Some(err), - VortexError::IOError(err, _) => Some(err), + FlatBuffers(err, _) => Some(err), + Io(err, _) => Some(err), #[cfg(feature = "object_store")] - VortexError::ObjectStore(err, _) => Some(err), - VortexError::JiffError(err, _) => Some(err), + ObjectStore(err, _) => Some(err), + Jiff(err, _) => Some(err), #[cfg(feature = "tokio")] - VortexError::JoinError(err, _) => Some(err), - VortexError::UrlError(err, _) => Some(err), + Join(err, _) => Some(err), #[cfg(feature = "serde")] - VortexError::SerdeJsonError(err, _) => Some(err), - VortexError::Prost(err, _) => Some(err.as_ref()), + SerdeJson(err, _) => Some(err), + Prost(err, _) => Some(err.as_ref()), _ => None, } } } -impl Debug for VortexError { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - Display::fmt(self, f) - } -} - /// A type alias for Results that return VortexErrors as their error type. pub type VortexResult = Result; @@ -329,6 +388,13 @@ impl VortexExpect for Option { /// A convenient macro for creating a VortexError. #[macro_export] macro_rules! vortex_err { + (Other: $($tts:tt)*) => {{ + use std::backtrace::Backtrace; + let err_string = format!($($tts)*); + $crate::__private::must_use( + $crate::VortexError::Other(err_string.into(), Box::new(Backtrace::capture())) + ) + }}; (AssertionFailed: $($tts:tt)*) => {{ use std::backtrace::Backtrace; let err_string = format!($($tts)*); @@ -371,6 +437,12 @@ macro_rules! vortex_err { $crate::VortexError::Context($msg.into(), Box::new($err)) ) }}; + (External: $err:expr) => {{ + use std::backtrace::Backtrace; + $crate::__private::must_use( + $crate::VortexError::External($err.into(), Box::new(Backtrace::capture())) + ) + }}; ($variant:ident: $fmt:literal $(, $arg:expr)* $(,)?) => {{ use std::backtrace::Backtrace; $crate::__private::must_use( @@ -383,11 +455,11 @@ macro_rules! vortex_err { ) }; ($fmt:literal $(, $arg:expr)* $(,)?) => { - $crate::vortex_err!(InvalidArgument: $fmt, $($arg),*) + $crate::vortex_err!(Other: $fmt, $($arg),*) }; } -/// A convenient macro for returning a VortexError. +/// A convenience macro for returning a VortexError. #[macro_export] macro_rules! vortex_bail { ($($tt:tt)+) => { @@ -460,26 +532,26 @@ macro_rules! vortex_panic { impl From for VortexError { fn from(value: arrow_schema::ArrowError) -> Self { - VortexError::ArrowError(value, Box::new(Backtrace::capture())) + VortexError::Arrow(value, Box::new(Backtrace::capture())) } } #[cfg(feature = "flatbuffers")] impl From for VortexError { fn from(value: flatbuffers::InvalidFlatbuffer) -> Self { - VortexError::FlatBuffersError(value, Box::new(Backtrace::capture())) + VortexError::FlatBuffers(value, Box::new(Backtrace::capture())) } } impl From for VortexError { fn from(value: io::Error) -> Self { - VortexError::IOError(value, Box::new(Backtrace::capture())) + VortexError::Io(value, Box::new(Backtrace::capture())) } } impl From for VortexError { fn from(value: std::array::TryFromSliceError) -> Self { - VortexError::TryFromSliceError(value, Box::new(Backtrace::capture())) + VortexError::TryFromSlice(value, Box::new(Backtrace::capture())) } } @@ -492,7 +564,7 @@ impl From for VortexError { impl From for VortexError { fn from(value: jiff::Error) -> Self { - VortexError::JiffError(value, Box::new(Backtrace::capture())) + VortexError::Jiff(value, Box::new(Backtrace::capture())) } } @@ -502,17 +574,11 @@ impl From for VortexError { if value.is_panic() { std::panic::resume_unwind(value.into_panic()) } else { - VortexError::JoinError(value, Box::new(Backtrace::capture())) + VortexError::Join(value, Box::new(Backtrace::capture())) } } } -impl From for VortexError { - fn from(value: url::ParseError) -> Self { - VortexError::UrlError(value, Box::new(Backtrace::capture())) - } -} - impl From for VortexError { fn from(value: TryFromIntError) -> Self { VortexError::TryFromInt(value, Box::new(Backtrace::capture())) @@ -522,7 +588,7 @@ impl From for VortexError { #[cfg(feature = "serde")] impl From for VortexError { fn from(value: serde_json::Error) -> Self { - VortexError::SerdeJsonError(value, Box::new(Backtrace::capture())) + VortexError::SerdeJson(value, Box::new(Backtrace::capture())) } } diff --git a/vortex-error/tests/fmt.rs b/vortex-error/tests/fmt.rs new file mode 100644 index 00000000000..bc92c543ba3 --- /dev/null +++ b/vortex-error/tests/fmt.rs @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +// This seems to be a bug in the lint - https://github.com/rust-lang/rust-clippy/issues/11024 +#![allow(clippy::tests_outside_test_module)] + +use serial_test::serial; +use vortex_error::VortexError; +use vortex_error::vortex_err; + +#[test] +#[serial] +fn test_basic_display() { + temp_env::with_var("RUST_BACKTRACE", Some("1"), || { + let err = vortex_err!("this is bad"); + let display = err.to_string(); + + assert!( + display.contains("Other error: this is bad"), + "should contain error message" + ); + assert!( + display.contains("Backtrace:"), + "should contain backtrace header" + ); + assert!( + display.contains("test_basic_display"), + "backtrace should include test function" + ); + }); +} + +#[test] +#[serial] +fn test_from_arrow_with_backtrace() { + temp_env::with_var("RUST_BACKTRACE", Some("1"), || { + let arrow_error = arrow_schema::ArrowError::NotYetImplemented( + "This feature isn't implemented yet".to_string(), + ); + + let vx_error = VortexError::from(arrow_error); + let display = vx_error.to_string(); + + assert!( + display + .contains("Arrow error: Not yet implemented: This feature isn't implemented yet"), + "should contain arrow error message" + ); + assert!( + display.contains("Backtrace:"), + "should contain backtrace header" + ); + }); +} + +#[test] +#[serial] +fn test_from_arrow_no_backtrace() { + // Detect a nextest run, because `Backtrace::capture` caches whether backtraces are enabled + // and `cargo test` runs tests in the same process, while nextest uses separate processes. + if std::env::var("NEXTEST_RUN_ID").is_ok() { + temp_env::with_var("RUST_BACKTRACE", Some("0"), || { + let arrow_error = arrow_schema::ArrowError::NotYetImplemented( + "This feature isn't implemented yet".to_string(), + ); + + let vx_error = VortexError::from(arrow_error); + let display = vx_error.to_string(); + + assert!( + display.contains( + "Arrow error: Not yet implemented: This feature isn't implemented yet" + ), + "should contain arrow error message" + ); + assert!( + !display.contains("Backtrace:"), + "should not contain backtrace when RUST_BACKTRACE=0" + ); + }); + } +} diff --git a/vortex-jni/src/file.rs b/vortex-jni/src/file.rs index b721d1ec746..2ab70aa5e85 100644 --- a/vortex-jni/src/file.rs +++ b/vortex-jni/src/file.rs @@ -159,7 +159,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFileMethods_delete<'local>( } // Pick the first URL to use for building the client - let store_url = Url::parse(&delete_uris[0]).map_err(VortexError::from)?; + let store_url = Url::parse(&delete_uris[0]).map_err(|e| vortex_err!(External: e))?; let mut properties: HashMap = HashMap::new(); @@ -178,7 +178,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFileMethods_delete<'local>( let (store, _) = make_object_store(&store_url, &properties)?; for uri in delete_uris { - let url = Url::parse(&uri).map_err(VortexError::from)?; + let url = Url::parse(&uri).map_err(|e| vortex_err!(External: e))?; // TODO(aduffy): block on all of them TOKIO_RUNTIME .block_on( diff --git a/vortex-scalar/src/proto.rs b/vortex-scalar/src/proto.rs index 1b41b0af85b..d6cee0b9366 100644 --- a/vortex-scalar/src/proto.rs +++ b/vortex-scalar/src/proto.rs @@ -183,14 +183,14 @@ impl Scalar { value .dtype .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "Scalar missing dtype"))?, + .ok_or_else(|| vortex_err!(Serde: "Scalar missing dtype"))?, session, )?; let pb_scalar_value: &pb::ScalarValue = value .value .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "Scalar missing value"))?; + .ok_or_else(|| vortex_err!(Serde: "Scalar missing value"))?; let value: Option = ScalarValue::from_proto(pb_scalar_value, &dtype)?; @@ -224,7 +224,7 @@ impl ScalarValue { let kind = value .kind .as_ref() - .ok_or_else(|| vortex_err!(InvalidSerde: "Scalar value missing kind"))?; + .ok_or_else(|| vortex_err!(Serde: "Scalar value missing kind"))?; // `DType::Extension` store their serialized values using the storage `DType`. let dtype = match dtype { @@ -251,7 +251,7 @@ impl ScalarValue { fn bool_from_proto(v: bool, dtype: &DType) -> VortexResult { vortex_ensure!( dtype.is_boolean(), - InvalidSerde: "expected Bool dtype for BoolValue, got {dtype}" + Serde: "expected Bool dtype for BoolValue, got {dtype}" ); Ok(ScalarValue::Bool(v)) @@ -264,7 +264,7 @@ fn bool_from_proto(v: bool, dtype: &DType) -> VortexResult { fn int64_from_proto(v: i64, dtype: &DType) -> VortexResult { vortex_ensure!( dtype.is_primitive(), - InvalidSerde: "expected Primitive dtype for Int64Value, got {dtype}" + Serde: "expected Primitive dtype for Int64Value, got {dtype}" ); let pvalue = match dtype.as_ptype() { @@ -273,10 +273,10 @@ fn int64_from_proto(v: i64, dtype: &DType) -> VortexResult { PType::I32 => v.to_i32().map(PValue::I32), PType::I64 => Some(PValue::I64(v)), ptype => vortex_bail!( - InvalidSerde: "expected signed integer ptype for Int64Value, got {ptype}" + Serde: "expected signed integer ptype for Int64Value, got {ptype}" ), } - .ok_or_else(|| vortex_err!(InvalidSerde: "Int64 value {v} out of range for dtype {dtype}"))?; + .ok_or_else(|| vortex_err!(Serde: "Int64 value {v} out of range for dtype {dtype}"))?; Ok(ScalarValue::Primitive(pvalue)) } @@ -289,7 +289,7 @@ fn int64_from_proto(v: i64, dtype: &DType) -> VortexResult { fn uint64_from_proto(v: u64, dtype: &DType) -> VortexResult { vortex_ensure!( dtype.is_primitive(), - InvalidSerde: "expected Primitive dtype for Uint64Value, got {dtype}" + Serde: "expected Primitive dtype for Uint64Value, got {dtype}" ); let pvalue = match dtype.as_ptype() { @@ -300,10 +300,10 @@ fn uint64_from_proto(v: u64, dtype: &DType) -> VortexResult { // Backwards compatibility: f16 values were previously serialized as u64. PType::F16 => v.to_u16().map(f16::from_bits).map(PValue::F16), ptype => vortex_bail!( - InvalidSerde: "expected unsigned integer ptype for Uint64Value, got {ptype}" + Serde: "expected unsigned integer ptype for Uint64Value, got {ptype}" ), } - .ok_or_else(|| vortex_err!(InvalidSerde: "Uint64 value {v} out of range for dtype {dtype}"))?; + .ok_or_else(|| vortex_err!(Serde: "Uint64 value {v} out of range for dtype {dtype}"))?; Ok(ScalarValue::Primitive(pvalue)) } @@ -312,12 +312,11 @@ fn uint64_from_proto(v: u64, dtype: &DType) -> VortexResult { fn f16_from_proto(v: u64, dtype: &DType) -> VortexResult { vortex_ensure!( matches!(dtype, DType::Primitive(PType::F16, _)), - InvalidSerde: "expected F16 dtype for F16Value, got {dtype}" + Serde: "expected F16 dtype for F16Value, got {dtype}" ); - let bits = u16::try_from(v).map_err( - |_| vortex_err!(InvalidSerde: "f16 bitwise representation has more than 16 bits: {v}"), - )?; + let bits = u16::try_from(v) + .map_err(|_| vortex_err!(Serde: "f16 bitwise representation has more than 16 bits: {v}"))?; Ok(ScalarValue::Primitive(PValue::F16(f16::from_bits(bits)))) } @@ -326,7 +325,7 @@ fn f16_from_proto(v: u64, dtype: &DType) -> VortexResult { fn f32_from_proto(v: f32, dtype: &DType) -> VortexResult { vortex_ensure!( matches!(dtype, DType::Primitive(PType::F32, _)), - InvalidSerde: "expected F32 dtype for F32Value, got {dtype}" + Serde: "expected F32 dtype for F32Value, got {dtype}" ); Ok(ScalarValue::Primitive(PValue::F32(v))) @@ -336,7 +335,7 @@ fn f32_from_proto(v: f32, dtype: &DType) -> VortexResult { fn f64_from_proto(v: f64, dtype: &DType) -> VortexResult { vortex_ensure!( matches!(dtype, DType::Primitive(PType::F64, _)), - InvalidSerde: "expected F64 dtype for F64Value, got {dtype}" + Serde: "expected F64 dtype for F64Value, got {dtype}" ); Ok(ScalarValue::Primitive(PValue::F64(v))) @@ -349,7 +348,7 @@ fn string_from_proto(s: &str, dtype: &DType) -> VortexResult { DType::Utf8(_) => Ok(ScalarValue::Utf8(BufferString::from(s))), DType::Binary(_) => Ok(ScalarValue::Binary(ByteBuffer::copy_from(s.as_bytes()))), _ => vortex_bail!( - InvalidSerde: "expected Utf8 or Binary dtype for StringValue, got {dtype}" + Serde: "expected Utf8 or Binary dtype for StringValue, got {dtype}" ), } } @@ -370,19 +369,19 @@ fn bytes_from_proto(bytes: &[u8], dtype: &DType) -> VortexResult { 8 => DecimalValue::I64(i64::from_le_bytes(bytes.try_into()?)), 16 => DecimalValue::I128(i128::from_le_bytes(bytes.try_into()?)), 32 => DecimalValue::I256(i256::from_le_bytes(bytes.try_into()?)), - l => vortex_bail!(InvalidSerde: "invalid decimal byte length: {l}"), + l => vortex_bail!(Serde: "invalid decimal byte length: {l}"), })), _ => vortex_bail!( - InvalidSerde: "expected Utf8, Binary, or Decimal dtype for BytesValue, got {dtype}" + Serde: "expected Utf8, Binary, or Decimal dtype for BytesValue, got {dtype}" ), } } /// Deserialize a [`ScalarValue::List`] from a protobuf `ListValue`. fn list_from_proto(v: &ListValue, dtype: &DType) -> VortexResult { - let element_dtype = dtype.as_list_element_opt().ok_or_else( - || vortex_err!(InvalidSerde: "expected List dtype for ListValue, got {dtype}"), - )?; + let element_dtype = dtype + .as_list_element_opt() + .ok_or_else(|| vortex_err!(Serde: "expected List dtype for ListValue, got {dtype}"))?; let mut values = Vec::with_capacity(v.values.len()); for elem in v.values.iter() { diff --git a/vortex-tui/src/convert.rs b/vortex-tui/src/convert.rs index 858e908ec2e..240ed34d8fa 100644 --- a/vortex-tui/src/convert.rs +++ b/vortex-tui/src/convert.rs @@ -17,8 +17,8 @@ use vortex::array::arrow::FromArrowArray; use vortex::array::stream::ArrayStreamAdapter; use vortex::dtype::DType; use vortex::dtype::arrow::FromArrowType; -use vortex::error::VortexError; use vortex::error::VortexExpect; +use vortex::error::vortex_err; use vortex::file::WriteOptionsSessionExt; use vortex::file::WriteStrategyBuilder; use vortex::session::VortexSession; @@ -75,7 +75,7 @@ pub async fn exec_convert(session: &VortexSession, flags: ConvertArgs) -> anyhow .build()? .map(|record_batch| { record_batch - .map_err(|e| VortexError::generic(e.into())) + .map_err(|e| vortex_err!(External: e)) .and_then(|rb| ArrayRef::from_arrow(rb, false)) }) .boxed(); diff --git a/vortex-vector/src/primitive/cast.rs b/vortex-vector/src/primitive/cast.rs index 97ac1b580c8..6805e34a481 100644 --- a/vortex-vector/src/primitive/cast.rs +++ b/vortex-vector/src/primitive/cast.rs @@ -38,7 +38,7 @@ pub fn cast_pvector( let mut buffer = BufferMut::with_capacity(elements.len()); for &item in elements { let converted = ::from(item).ok_or_else( - || vortex_err!(ComputeError: "Failed to cast {} to {:?}", item, Dst::PTYPE), + || vortex_err!(Compute: "Failed to cast {} to {:?}", item, Dst::PTYPE), )?; // SAFETY: We pre-allocated the required capacity. unsafe { buffer.push_unchecked(converted) } @@ -54,7 +54,7 @@ pub fn cast_pvector( for (&item, valid) in elements.iter().zip(bit_buffer.iter()) { if valid { let converted = ::from(item).ok_or_else( - || vortex_err!(ComputeError: "Failed to cast {} to {:?}", item, Dst::PTYPE), + || vortex_err!(Compute: "Failed to cast {} to {:?}", item, Dst::PTYPE), )?; // SAFETY: We pre-allocated the required capacity. unsafe { buffer.push_unchecked(converted) }