Skip to content

8759: AvroError enum for arrow-avro crate #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 4 commits into
base: main
Choose a base branch
from
Open

8759: AvroError enum for arrow-avro crate #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5c1b54
adds AvroError variant to arrow-avro crate
nathaniel-d-ef Oct 30, 2025
3ac8f87
Replace `ArrowError` with `AvroError` and adjust error handling throu…
nathaniel-d-ef Oct 31, 2025
6fb7248
Cleanup, apply suggestions
nathaniel-d-ef Nov 3, 2025
b40bb9c
Remove redundant newline
nathaniel-d-ef Nov 3, 2025
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
146 changes: 146 additions & 0 deletions arrow-avro/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

//! Common Avro errors and macros.

use arrow_schema::ArrowError;
use core::num::TryFromIntError;
use std::error::Error;
use std::string::FromUtf8Error;
use std::{io, result, str};

/// Avro error enumeration

#[derive(Debug)]
#[non_exhaustive]
pub enum AvroError {
/// General Avro error.
/// Returned when code violates normal workflow of working with Avro data.
General(String),
/// "Not yet implemented" Avro error.
/// Returned when functionality is not yet available.
NYI(String),
/// "End of file" Avro error.
/// Returned when IO related failures occur, e.g. when there are not enough bytes to
/// decode.
EOF(String),
/// Arrow error.
/// Returned when reading into arrow or writing from arrow.
ArrowError(Box<ArrowError>),
/// Error when the requested index is more than the
/// number of items expected
IndexOutOfBound(usize, usize),
/// Error indicating that an unexpected or bad argument was passed to a function.
InvalidArgument(String),
/// Error indicating that a value could not be parsed.
ParseError(String),
/// Error indicating that a schema is invalid.
SchemaError(String),
/// An external error variant
External(Box<dyn Error + Send + Sync>),
/// Returned when a function needs more data to complete properly. The `usize` field indicates
/// the total number of bytes required, not the number of additional bytes.
NeedMoreData(usize),
/// Returned when a function needs more data to complete properly.
/// The `Range<u64>` indicates the range of bytes that are needed.
NeedMoreDataRange(std::ops::Range<u64>),
}

impl std::fmt::Display for AvroError {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match &self {
AvroError::General(message) => {
write!(fmt, "Avro error: {message}")
}
AvroError::NYI(message) => write!(fmt, "NYI: {message}"),
AvroError::EOF(message) => write!(fmt, "EOF: {message}"),
AvroError::ArrowError(message) => write!(fmt, "Arrow: {message}"),
AvroError::IndexOutOfBound(index, bound) => {
write!(fmt, "Index {index} out of bound: {bound}")
}
AvroError::InvalidArgument(message) => {
write!(fmt, "Invalid argument: {message}")
}
AvroError::ParseError(message) => write!(fmt, "Parse error: {message}"),
AvroError::SchemaError(message) => write!(fmt, "Schema error: {message}"),
AvroError::External(e) => write!(fmt, "External: {e}"),
AvroError::NeedMoreData(needed) => write!(fmt, "NeedMoreData: {needed}"),
AvroError::NeedMoreDataRange(range) => {
write!(fmt, "NeedMoreDataRange: {}..{}", range.start, range.end)
}
}
}
}

impl Error for AvroError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
match self {
AvroError::External(e) => Some(e.as_ref()),
AvroError::ArrowError(e) => Some(e.as_ref()),
_ => None,
}
}
}

impl From<TryFromIntError> for AvroError {
fn from(e: TryFromIntError) -> AvroError {
AvroError::General(format!("Integer overflow: {e}"))
}
}

impl From<io::Error> for AvroError {
fn from(e: io::Error) -> AvroError {
AvroError::External(Box::new(e))
}
}

impl From<str::Utf8Error> for AvroError {
fn from(e: str::Utf8Error) -> AvroError {
AvroError::External(Box::new(e))
}
}

impl From<FromUtf8Error> for AvroError {
fn from(e: FromUtf8Error) -> AvroError {
AvroError::External(Box::new(e))
}
}

impl From<ArrowError> for AvroError {
fn from(e: ArrowError) -> Self {
AvroError::ArrowError(Box::new(e))
}
}

/// A specialized `Result` for Avro errors.
pub type Result<T, E = AvroError> = result::Result<T, E>;

impl From<AvroError> for io::Error {
fn from(e: AvroError) -> Self {
io::Error::other(e)
}
}

impl From<AvroError> for ArrowError {
fn from(e: AvroError) -> Self {
match e {
AvroError::External(inner) => ArrowError::from_external_error(inner),
AvroError::ArrowError(inner) => ArrowError::from_external_error(inner),
Copy link

@cursor cursor bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Avro Error Conversion: Prevent Redundant Wrapping

When converting AvroError::ArrowError(inner) to ArrowError, the code calls ArrowError::from_external_error(inner) which wraps the boxed ArrowError as an external error. This creates unnecessary error nesting and loses the original error type. The correct approach is to unbox and return the ArrowError directly with *inner instead of wrapping it as an external error.

Fix in Cursor Fix in Web

Copy link
Owner Author

@martin-augment martin-augment Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct that there is no need to wrap an ArrowError in another ArrowError. Prevents a needless allocation and duplication.

other => ArrowError::AvroError(other.to_string()),
}
Comment on lines +138 to +144
Copy link

@coderabbitai coderabbitai bot Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preserve original ArrowError variant when converting back

AvroError::ArrowError(inner) currently re-emits as ArrowError::ExternalError, so any downstream code that previously matched on precise ArrowError variants (e.g., SchemaError, ComputeError) will now only see ExternalError, breaking existing error handling. Just return the boxed ArrowError you already own to keep behavior identical to the pre-refactor code.

 impl From<AvroError> for ArrowError {
     fn from(e: AvroError) -> Self {
         match e {
             AvroError::External(inner) => ArrowError::from_external_error(inner),
-            AvroError::ArrowError(inner) => ArrowError::from_external_error(inner),
+            AvroError::ArrowError(inner) => *inner,
             other => ArrowError::AvroError(other.to_string()),
         }
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
impl From<AvroError> for ArrowError {
fn from(e: AvroError) -> Self {
match e {
AvroError::External(inner) => ArrowError::from_external_error(inner),
AvroError::ArrowError(inner) => ArrowError::from_external_error(inner),
other => ArrowError::AvroError(other.to_string()),
}
impl From<AvroError> for ArrowError {
fn from(e: AvroError) -> Self {
match e {
AvroError::External(inner) => ArrowError::from_external_error(inner),
AvroError::ArrowError(inner) => *inner,
other => ArrowError::AvroError(other.to_string()),
}
}
}
🤖 Prompt for AI Agents 
In arrow-avro/src/errors.rs around lines 138 to 144, the
AvroError::ArrowError(inner) arm currently re-wraps the boxed ArrowError as an
ExternalError; change it to return the original boxed ArrowError (unbox/return
the inner ArrowError directly) so existing downstream pattern matches
(SchemaError, ComputeError, etc.) continue to work unchanged.

Copy link
Owner Author

@martin-augment martin-augment Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct that there is no need to wrap an ArrowError in another ArrowError. Prevents a needless allocation and duplication.

Copy link

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-augment, thank you for confirming! I'm glad the analysis is helpful. The suggestion to return *inner directly will indeed avoid the unnecessary re-wrapping and preserve the original ArrowError variant for downstream consumers. Feel free to apply the suggested change when you're ready to address it.

}
}
3 changes: 3 additions & 0 deletions arrow-avro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ pub mod compression;
/// Avro data types and Arrow data types.
pub mod codec;

/// AvroError variants
pub mod errors;

/// Extension trait for AvroField to add Utf8View support
///
/// This trait adds methods for working with Utf8View support to the AvroField struct.
Expand Down
10 changes: 4 additions & 6 deletions arrow-avro/src/reader/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@

//! Decoder for [`Block`]

use crate::errors::{AvroError, Result};
use crate::reader::vlq::VLQDecoder;
use arrow_schema::ArrowError;

/// A file data block
///
Expand Down Expand Up @@ -75,14 +75,14 @@ impl BlockDecoder {
/// can then be used again to read the next block, if any
///
/// [`BufRead::fill_buf`]: std::io::BufRead::fill_buf
pub fn decode(&mut self, mut buf: &[u8]) -> Result<usize, ArrowError> {
pub fn decode(&mut self, mut buf: &[u8]) -> Result<usize> {
let max_read = buf.len();
while !buf.is_empty() {
match self.state {
BlockDecoderState::Count => {
if let Some(c) = self.vlq_decoder.long(&mut buf) {
self.in_progress.count = c.try_into().map_err(|_| {
ArrowError::ParseError(format!(
AvroError::ParseError(format!(
"Block count cannot be negative, got {c}"
))
})?;
Expand All @@ -93,9 +93,7 @@ impl BlockDecoder {
BlockDecoderState::Size => {
if let Some(c) = self.vlq_decoder.long(&mut buf) {
self.bytes_remaining = c.try_into().map_err(|_| {
ArrowError::ParseError(format!(
"Block size cannot be negative, got {c}"
))
AvroError::ParseError(format!("Block size cannot be negative, got {c}"))
})?;

self.in_progress.data.reserve(self.bytes_remaining);
Expand Down
51 changes: 22 additions & 29 deletions arrow-avro/src/reader/cursor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
// specific language governing permissions and limitations
// under the License.

use crate::errors::{AvroError, Result};
use crate::reader::vlq::read_varint;
use arrow_schema::ArrowError;

/// A wrapper around a byte slice, providing low-level decoding for Avro
///
Expand All @@ -43,88 +43,81 @@ impl<'a> AvroCursor<'a> {

/// Read a single `u8`
#[inline]
pub(crate) fn get_u8(&mut self) -> Result<u8, ArrowError> {
pub(crate) fn get_u8(&mut self) -> Result<u8> {
match self.buf.first().copied() {
Some(x) => {
self.buf = &self.buf[1..];
Ok(x)
}
None => Err(ArrowError::ParseError("Unexpected EOF".to_string())),
None => Err(AvroError::EOF("Unexpected EOF".to_string())),
}
}

#[inline]
pub(crate) fn get_bool(&mut self) -> Result<bool, ArrowError> {
pub(crate) fn get_bool(&mut self) -> Result<bool> {
Ok(self.get_u8()? != 0)
}

pub(crate) fn read_vlq(&mut self) -> Result<u64, ArrowError> {
let (val, offset) = read_varint(self.buf)
.ok_or_else(|| ArrowError::ParseError("bad varint".to_string()))?;
pub(crate) fn read_vlq(&mut self) -> Result<u64> {
let (val, offset) =
read_varint(self.buf).ok_or_else(|| AvroError::ParseError("bad varint".to_string()))?;
self.buf = &self.buf[offset..];
Ok(val)
}

#[inline]
pub(crate) fn get_int(&mut self) -> Result<i32, ArrowError> {
pub(crate) fn get_int(&mut self) -> Result<i32> {
let varint = self.read_vlq()?;
let val: u32 = varint
.try_into()
.map_err(|_| ArrowError::ParseError("varint overflow".to_string()))?;
.map_err(|_| AvroError::ParseError("varint overflow".to_string()))?;
Ok((val >> 1) as i32 ^ -((val & 1) as i32))
}

#[inline]
pub(crate) fn get_long(&mut self) -> Result<i64, ArrowError> {
pub(crate) fn get_long(&mut self) -> Result<i64> {
let val = self.read_vlq()?;
Ok((val >> 1) as i64 ^ -((val & 1) as i64))
}

pub(crate) fn get_bytes(&mut self) -> Result<&'a [u8], ArrowError> {
let len: usize = self.get_long()?.try_into().map_err(|_| {
ArrowError::ParseError("offset overflow reading avro bytes".to_string())
})?;
pub(crate) fn get_bytes(&mut self) -> Result<&'a [u8]> {
let len: usize = self
.get_long()?
.try_into()
.map_err(|_| AvroError::ParseError("offset overflow reading avro bytes".to_string()))?;

if self.buf.len() < len {
return Err(ArrowError::ParseError(
"Unexpected EOF reading bytes".to_string(),
));
return Err(AvroError::EOF("Unexpected EOF reading bytes".to_string()));
}
let ret = &self.buf[..len];
self.buf = &self.buf[len..];
Ok(ret)
}

#[inline]
pub(crate) fn get_float(&mut self) -> Result<f32, ArrowError> {
pub(crate) fn get_float(&mut self) -> Result<f32> {
if self.buf.len() < 4 {
return Err(ArrowError::ParseError(
"Unexpected EOF reading float".to_string(),
));
return Err(AvroError::EOF("Unexpected EOF reading float".to_string()));
}
let ret = f32::from_le_bytes(self.buf[..4].try_into().unwrap());
self.buf = &self.buf[4..];
Ok(ret)
}

#[inline]
pub(crate) fn get_double(&mut self) -> Result<f64, ArrowError> {
pub(crate) fn get_double(&mut self) -> Result<f64> {
if self.buf.len() < 8 {
return Err(ArrowError::ParseError(
"Unexpected EOF reading float".to_string(),
));
return Err(AvroError::EOF("Unexpected EOF reading float".to_string()));
}
let ret = f64::from_le_bytes(self.buf[..8].try_into().unwrap());
self.buf = &self.buf[8..];
Ok(ret)
}

/// Read exactly `n` bytes from the buffer (e.g. for Avro `fixed`).
pub(crate) fn get_fixed(&mut self, n: usize) -> Result<&'a [u8], ArrowError> {
pub(crate) fn get_fixed(&mut self, n: usize) -> Result<&'a [u8]> {
if self.buf.len() < n {
return Err(ArrowError::ParseError(
"Unexpected EOF reading fixed".to_string(),
));
return Err(AvroError::EOF("Unexpected EOF reading fixed".to_string()));
}
let ret = &self.buf[..n];
self.buf = &self.buf[n..];
Expand Down
20 changes: 10 additions & 10 deletions arrow-avro/src/reader/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
//! Decoder for [`Header`]

use crate::compression::{CODEC_METADATA_KEY, CompressionCodec};
use crate::errors::{AvroError, Result};
use crate::reader::vlq::VLQDecoder;
use crate::schema::{SCHEMA_METADATA_KEY, Schema};
use arrow_schema::ArrowError;
use std::io::BufRead;

/// Read the Avro file header (magic, metadata, sync marker) from `reader`.
pub(crate) fn read_header<R: BufRead>(mut reader: R) -> Result<Header, ArrowError> {
pub(crate) fn read_header<R: BufRead>(mut reader: R) -> Result<Header> {
let mut decoder = HeaderDecoder::default();
loop {
let buf = reader.fill_buf()?;
Expand All @@ -39,7 +39,7 @@ pub(crate) fn read_header<R: BufRead>(mut reader: R) -> Result<Header, ArrowErro
}
}
decoder.flush().ok_or_else(|| {
ArrowError::ParseError("Unexpected EOF while reading Avro header".to_string())
AvroError::ParseError("Unexpected EOF while reading Avro header".to_string())
})
}

Expand Down Expand Up @@ -96,7 +96,7 @@ impl Header {
}

/// Returns the [`CompressionCodec`] if any
pub fn compression(&self) -> Result<Option<CompressionCodec>, ArrowError> {
pub fn compression(&self) -> Result<Option<CompressionCodec>> {
let v = self.get(CODEC_METADATA_KEY);
match v {
None | Some(b"null") => Ok(None),
Expand All @@ -105,19 +105,19 @@ impl Header {
Some(b"zstandard") => Ok(Some(CompressionCodec::ZStandard)),
Some(b"bzip2") => Ok(Some(CompressionCodec::Bzip2)),
Some(b"xz") => Ok(Some(CompressionCodec::Xz)),
Some(v) => Err(ArrowError::ParseError(format!(
Some(v) => Err(AvroError::ParseError(format!(
"Unrecognized compression codec \'{}\'",
String::from_utf8_lossy(v)
))),
}
}

/// Returns the `Schema` if any
pub(crate) fn schema(&self) -> Result<Option<Schema<'_>>, ArrowError> {
pub(crate) fn schema(&self) -> Result<Option<Schema<'_>>> {
self.get(SCHEMA_METADATA_KEY)
.map(|x| {
serde_json::from_slice(x).map_err(|e| {
ArrowError::ParseError(format!("Failed to parse Avro schema JSON: {e}"))
AvroError::ParseError(format!("Failed to parse Avro schema JSON: {e}"))
})
})
.transpose()
Expand Down Expand Up @@ -175,15 +175,15 @@ impl HeaderDecoder {
/// input bytes, and the header can be obtained with [`Self::flush`]
///
/// [`BufRead::fill_buf`]: std::io::BufRead::fill_buf
pub fn decode(&mut self, mut buf: &[u8]) -> Result<usize, ArrowError> {
pub fn decode(&mut self, mut buf: &[u8]) -> Result<usize> {
let max_read = buf.len();
while !buf.is_empty() {
match self.state {
HeaderDecoderState::Magic => {
let remaining = &MAGIC[MAGIC.len() - self.bytes_remaining..];
let to_decode = buf.len().min(remaining.len());
if !buf.starts_with(&remaining[..to_decode]) {
return Err(ArrowError::ParseError("Incorrect avro magic".to_string()));
return Err(AvroError::ParseError("Incorrect avro magic".to_string()));
}
self.bytes_remaining -= to_decode;
buf = &buf[to_decode..];
Expand Down Expand Up @@ -310,7 +310,7 @@ mod test {
let mut decoder = HeaderDecoder::default();
decoder.decode(b"Ob").unwrap();
let err = decoder.decode(b"s").unwrap_err().to_string();
assert_eq!(err, "Parser error: Incorrect avro magic");
assert_eq!(err, "Parse error: Incorrect avro magic");
}

fn decode_file(file: &str) -> Header {
Expand Down
Loading