Copilot commented on code in PR #503:
URL: https://github.com/apache/hudi-rs/pull/503#discussion_r2656603885
##########
crates/core/src/avro_to_arrow/arrow_array_reader.rs:
##########
@@ -856,11 +857,11 @@ fn resolve_string(v: &Value) ->
ArrowResult<Option<String>> {
match v {
Value::String(s) => Ok(Some(s.clone())),
Value::Bytes(bytes) => String::from_utf8(bytes.to_vec())
- .map_err(AvroError::ConvertToUtf8)
+ .map_err(|e| AvroError::new(AvroDetails::ConvertToUtf8(e)))
.map(Some),
Value::Enum(_, s) => Ok(Some(s.clone())),
Value::Null => Ok(None),
- other => Err(AvroError::GetString(other.into())),
+ other => Err(AvroError::new(AvroDetails::GetString(other.clone()))),
Review Comment:
The use of `other.clone()` creates an unnecessary copy of the Avro Value.
Since this is in an error path, the value is only needed for error reporting.
Consider if the new apache-avro API supports borrowing or if there's a more
efficient way to construct this error without cloning the entire Value
structure.
##########
crates/core/src/avro_to_arrow/arrow_array_reader.rs:
##########
@@ -892,7 +893,7 @@ fn resolve_bytes(v: &Value) -> Option<Vec<u8>> {
.collect::<Result<Vec<_>, _>>()
.ok()?,
)),
- other => Err(AvroError::GetBytes(other.into())),
+ other => Err(AvroError::new(AvroDetails::GetBytes(other.clone()))),
Review Comment:
The use of `other.clone()` creates an unnecessary copy of the Avro Value.
Since this is in an error path, the value is only needed for error reporting.
Consider if the new apache-avro API supports borrowing or if there's a more
efficient way to construct this error without cloning the entire Value
structure.
##########
crates/core/src/avro_to_arrow/arrow_array_reader.rs:
##########
@@ -869,15 +870,15 @@ fn resolve_u8(v: &Value) -> AvroResult<u8> {
let int = match v {
Value::Int(n) => Ok(Value::Int(*n)),
Value::Long(n) => Ok(Value::Int(*n as i32)),
- other => Err(AvroError::GetU8(other.into())),
+ other => Err(AvroError::new(AvroDetails::GetU8(other.clone()))),
Review Comment:
The use of `other.clone()` creates an unnecessary copy of the Avro Value.
Since this is in an error path, the value is only needed for error reporting.
Consider if the new apache-avro API supports borrowing or if there's a more
efficient way to construct this error without cloning the entire Value
structure.
```suggestion
// Avoid cloning the entire Avro Value for error reporting; use a
lightweight placeholder instead.
other => Err(AvroError::new(AvroDetails::GetU8(Value::Null))),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
