Skip to content

Commit 2087482

Browse files
robert3005claude
andcommitted
Audit the error kinds for consistency and for wraps that lose them
A pass over all 3183 tagged sites, with the compiler doing the parts a text scan cannot. Tags. 17 messages carried different kinds at different sites. The rule that settles each: a slot or buffer count is an InvalidArgument, never a type error; a child index nobody can satisfy is OutOfBounds, whether it is phrased as "invalid child index" or "has no child"; a check that a kernel produced the declared length is AssertionFailed; and metadata checks are Serde. 42 sites move, including the row-kernel input checks that were InvalidArgument in one file and AssertionFailed in the next, and the view bounds checks in VarBinView whose messages say "out of bounds". Four sites that arrived with the rebase get their kinds. Wraps. A `.map_err(|e| vortex_err!(Kind: "...: {e}"))` around a call that already returns a VortexError replaces its kind with the wrapper's guess and nests its whole Display, backtrace included, inside the new message. No text scan can tell those from the same shape around an io::Error, so a throwaway build probed every such closure with a marker type whose must_use warning fires only when the wrapped value is a VortexError. It found 10: the three statistics conversions, DType and union-scalar deserialization, a CUDA scan column name, and four panics in test harnesses. Each now uses `with_context`, or the `vortex_panic!(err, "...")` form, which keep the kind. STYLE.md now says to do that. From impls. `From<ArrowError>` sent every Arrow failure to Other. Arrow's variants say what went wrong, so each now maps to its kind: NotYetImplemented to NotImplemented, ArithmeticOverflow and the three offset overflows to Overflow, SchemaError to MismatchedTypes, IoError to Io, the file-format variants to Serde, and a rejected cast, parse or argument to InvalidArgument. `From<jiff::Error>` maps a date that does not exist or a misused unit, which jiff exposes as `is_range` and `is_invalid_parameter`, to InvalidArgument. Both keep the original error as the source. case_when's `serialize` said Serde for what is an unimplemented operation; it is NotImplemented. Signed-off-by: Robert Kruszewski <github@robertk.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S7EtMqoZ91rrCCh9vS2yTm
1 parent 58a9cd5 commit 2087482

53 files changed

Lines changed: 172 additions & 90 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎STYLE.md‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,17 @@
5252
- `vortex_err!` for creating errors
5353
- `vortex_bail!` for returning errors
5454
- `vortex_panic!` for handling invariant violations
55-
- Add context to errors using `.with_context()`
55+
- Every message names its `VortexErrorKind`, as in `vortex_bail!(InvalidArgument: "...")`; the
56+
macros reject a message without one. Choose the kind by what went wrong, the way Python picks an
57+
exception class: `InvalidArgument` (`ValueError`) for a value the callee rejects,
58+
`MismatchedTypes` (`TypeError`) for the wrong dtype, `OutOfBounds` (`IndexError`) for a position
59+
past the end, `NotFound` (`KeyError`) for a name that resolves to nothing, `Overflow`
60+
(`OverflowError`) for a value that does not fit its target, `NotImplemented`, `Serde` for an
61+
unreadable wire or file format, `Io` for a failed system or driver call, `AssertionFailed` for a
62+
broken internal invariant, and `Other` only when nothing else fits. Never classify by where the
63+
failure happened.
64+
- Add context to an existing error with `.with_context()`, which keeps its kind; do not format it
65+
into a new `vortex_err!`, which discards the kind.
5666
- Include backtraces for better debugging
5767
- Use `VortexExpect` trait when unwrapping is appropriate with proper error context.
5868

‎encodings/alp/src/alp/array.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ impl ALPData {
310310
Ok(DType::Primitive(PType::F64, *nullability))
311311
}
312312
_ => {
313-
vortex_bail!(InvalidArgument: "ALP encoded ints have invalid DType {}", encoded.dtype(),)
313+
vortex_bail!(MismatchedTypes: "ALP encoded ints have invalid DType {}", encoded.dtype(),)
314314
}
315315
}
316316
}

‎encodings/bytebool/src/array.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ impl VTable for ByteBool {
145145
};
146146

147147
if buffers.len() != 1 {
148-
vortex_bail!(MismatchedTypes: "Expected 1 buffer, got {}", buffers.len());
148+
vortex_bail!(InvalidArgument: "Expected 1 buffer, got {}", buffers.len());
149149
}
150150
let buffer = buffers[0].clone();
151151

‎encodings/decimal-byte-parts/src/decimal_byte_parts/array.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ impl VTable for DecimalByteParts {
219219
slots.len()
220220
);
221221
for (idx, slot) in slots.iter().enumerate() {
222-
vortex_ensure!(slot.is_some(), NotFound: "missing required slot {idx}");
222+
vortex_ensure!(slot.is_some(), InvalidArgument: "missing required slot {idx}");
223223
}
224224

225225
let slots = DecimalBytePartsSlotsView::from_slots(slots);

‎encodings/fastlanes/src/bitpacking/vtable/mod.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ impl VTable for BitPacked {
188188
) -> VortexResult<ArrayParts<Self>> {
189189
let metadata = BitPackedMetadata::decode(metadata)?;
190190
if buffers.len() != 1 {
191-
vortex_bail!(MismatchedTypes: "Expected 1 buffer, got {}", buffers.len());
191+
vortex_bail!(InvalidArgument: "Expected 1 buffer, got {}", buffers.len());
192192
}
193193
let packed = buffers[0].clone();
194194

@@ -200,7 +200,7 @@ impl VTable for BitPacked {
200200
Ok(Validity::Array(validity))
201201
} else {
202202
vortex_bail!(
203-
MismatchedTypes: "Expected {} or {} children, got {}",
203+
InvalidArgument: "Expected {} or {} children, got {}",
204204
child_idx,
205205
child_idx + 1,
206206
children.len()

‎encodings/fastlanes/src/rle/probe.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ pub(crate) fn scalar_at(
3131
let (mut retained, mut children) = state.split();
3232
let code = children
3333
.slot(RLESlots::INDICES)?
34-
.ok_or_else(|| vortex_err!(NotFound: "RLE indices slot is missing"))?
34+
.ok_or_else(|| vortex_err!(InvalidArgument: "RLE indices slot is missing"))?
3535
.execute_scalar(logical_index, ctx)?;
3636
let Some(code) = code.as_primitive().as_::<usize>() else {
3737
return Ok(Scalar::null(array.dtype().clone()));
@@ -47,7 +47,7 @@ pub(crate) fn scalar_at(
4747
let value = read_offset(
4848
children
4949
.slot(RLESlots::VALUES_IDX_OFFSETS)?
50-
.ok_or_else(|| vortex_err!(NotFound: "RLE offsets slot is missing"))?
50+
.ok_or_else(|| vortex_err!(InvalidArgument: "RLE offsets slot is missing"))?
5151
.execute_scalar(0, ctx)?,
5252
)?;
5353
if let Some(state) = retained.as_mut() {
@@ -59,7 +59,7 @@ pub(crate) fn scalar_at(
5959
read_offset(
6060
children
6161
.slot(RLESlots::VALUES_IDX_OFFSETS)?
62-
.ok_or_else(|| vortex_err!(NotFound: "RLE offsets slot is missing"))?
62+
.ok_or_else(|| vortex_err!(InvalidArgument: "RLE offsets slot is missing"))?
6363
.execute_scalar(chunk, ctx)?,
6464
)?
6565
.checked_sub(base)
@@ -70,7 +70,7 @@ pub(crate) fn scalar_at(
7070
.ok_or_else(|| vortex_err!(Overflow: "RLE value index overflow"))?;
7171
let scalar = children
7272
.slot(RLESlots::VALUES)?
73-
.ok_or_else(|| vortex_err!(NotFound: "RLE values slot is missing"))?
73+
.ok_or_else(|| vortex_err!(InvalidArgument: "RLE values slot is missing"))?
7474
.execute_scalar(value_index, ctx)?;
7575
Scalar::try_new(array.dtype().clone(), scalar.into_value())
7676
}

‎encodings/parquet-variant/src/vtable.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ impl VTable for ParquetVariant {
220220
None => None,
221221
};
222222

223-
vortex_ensure!(matches!(dtype, DType::Variant(_)), InvalidArgument: "Expected Variant DType");
223+
vortex_ensure!(matches!(dtype, DType::Variant(_)), MismatchedTypes: "Expected Variant DType");
224224
let has_typed_value = typed_value_dtype.is_some();
225225
vortex_ensure!(
226226
proto.has_value || has_typed_value,

‎encodings/runend/src/probe.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ pub(crate) fn scalar_at(
2828
// between searches.
2929
let mut ends = state
3030
.slot(RunEndSlots::ENDS)?
31-
.ok_or_else(|| vortex_err!(NotFound: "RunEnd ends slot is missing"))?;
31+
.ok_or_else(|| vortex_err!(InvalidArgument: "RunEnd ends slot is missing"))?;
3232
let mut left = 0;
3333
let mut right = ends.array().len();
3434
while left < right {
@@ -42,7 +42,7 @@ pub(crate) fn scalar_at(
4242
}
4343
state
4444
.slot(RunEndSlots::VALUES)?
45-
.ok_or_else(|| vortex_err!(NotFound: "RunEnd values slot is missing"))?
45+
.ok_or_else(|| vortex_err!(InvalidArgument: "RunEnd values slot is missing"))?
4646
.execute_scalar(left, ctx)
4747
}
4848

‎encodings/sparse/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ impl VTable for Sparse {
269269
// Once we have the patches metadata, we need to get the fill value from the buffers.
270270

271271
if buffers.len() != 1 {
272-
vortex_bail!(MismatchedTypes: "Expected 1 buffer, got {}", buffers.len());
272+
vortex_bail!(InvalidArgument: "Expected 1 buffer, got {}", buffers.len());
273273
}
274274
let scalar_bytes: &[u8] = &buffers[0].clone().try_to_host_sync()?;
275275

‎encodings/zstd/src/array.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1537,7 +1537,7 @@ impl ZstdData {
15371537
}
15381538
}
15391539
}
1540-
_ => vortex_bail!(InvalidArgument: "Unsupported dtype for Zstd array: {}", dtype),
1540+
_ => vortex_bail!(MismatchedTypes: "Unsupported dtype for Zstd array: {}", dtype),
15411541
}
15421542
}
15431543

0 commit comments

Comments
 (0)