alamb commented on code in PR #2711:
URL: https://github.com/apache/arrow-rs/pull/2711#discussion_r970013668
##########
arrow-schema/src/schema.rs:
##########
@@ -205,13 +211,13 @@ impl Schema {
}
/// Find the index of the column with the given name.
- pub fn index_of(&self, name: &str) -> Result<usize> {
+ pub fn index_of(&self, name: &str) -> Result<usize, ArrowSchemaError> {
(0..self.fields.len())
.find(|idx| self.fields[*idx].name() == name)
.ok_or_else(|| {
let valid_fields: Vec<String> =
self.fields.iter().map(|f| f.name().clone()).collect();
- ArrowError::InvalidArgumentError(format!(
+ ArrowSchemaError::Field(format!(
Review Comment:
I think the error detail of the current `ArrowError` is more than sufficient
for all needs I have seen. I dislike both more structure as well as making more
different types of error enumerations as I believe it needlessly makes it
harder to both use and write the library code.
If we are going to mess with the error types in arrow-rs the only thing that
would excite me would be if they were changed to include context / source
information that was visible to users
##########
arrow-schema/src/error.rs:
##########
@@ -0,0 +1,45 @@
+// 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.
+
+//! Defines `ArrowSchemaError` for representing failures in arrow schema
+
+use std::error::Error;
+
+#[derive(Debug)]
+pub enum ArrowSchemaError {
Review Comment:
> The error variant is per-crate instead of a global shared enumeration. I
think this makes for better modularity, and more meaningful error variants - an
operation on a schema being able to return a ParquetError is a little odd imo
I have the opposite opinion here -- I have yet to see someone want to handle
one of the error variants specially -- as in have code that reacts differently
to an `ArrowSchemaError` vs a `ParquetError`, even though in theory someone
might want to.
I think a single global error variant:
1. has served us well in the Arrow and DataFusion crates and makes PRs
easier to contribute.
2. will avoid a lot of unnecessary downstream crate churn
IMO What is desperately needed in `ArrowError` (and Rust error handling more
generally) is a way to walk the error chain to see what paths lead to a
particular error.
So to be specific, I would rather see a crate `arrow-common` that has
`ArrowError` rather than new error variants added in all subcrates. .
Of course, this is just my opinion, and I would welcome what others think
(nothing like a good error handling discussion to inflame passions ;)).
--
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]
