You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/20 11:50:20 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #3574: Add external variant to ParquetError (#3285)
tustvold opened a new pull request, #3574:
URL: https://github.com/apache/arrow-rs/pull/3574
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3285
Relates to #2725
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082903996
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
That we don't actually provide a meaningful PartialEq implementation for a large portion of the errors we produce, I dunno it feels like a strange thing for an error to provide... I've double-checked that DataFusion doesn't provide something similar, neither do any of the other crates in this repo
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3574: Add external variant to ParquetError (#3285)
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#issuecomment-1399437884
Benchmark runs are scheduled for baseline = 24e5daef3248c38a6fb354c8427c9ba653e2b3e9 and contender = 892a80385bdc1ee4c033e3b848fed882982e352d. 892a80385bdc1ee4c033e3b848fed882982e352d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/02bccbcee3f846ae8635dc42ad0c55cf...3c7c8ebdf0444ba099e67e9a52403971/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/853f2a6183e94d64a8a7cf6af7f13c87...6d938732bf1a472da62b03f8ede93593/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/bc8ca1a011b94e32af947858b92cd279...68f56c9f89ad44028d46cf63d07f8933/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/140977e3461148cd89a254584dfc8e94...67af7381b0ba4fb5aedfe64a703759ce/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082867599
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
Hence the scream test :smile:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082574357
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
I worry that lulls people into a false sense of security, I would rather scream test removal and see if that's ok
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082866719
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
I guess @alamb means a downstream project may use `ParquetError` embedded in another error type? For that we cannot detect in our test.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3574: Add external variant to ParquetError (#3285)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082566616
##########
parquet/src/column/mod.rs:
##########
@@ -36,18 +36,18 @@
//! repetition levels and read them to verify write/read correctness.
//!
//! ```rust,no_run
-//! use std::{fs, path::Path, sync::Arc};
-//!
-//! use parquet::{
-//! column::{reader::ColumnReader, writer::ColumnWriter},
-//! data_type::Int32Type,
-//! file::{
-//! properties::WriterProperties,
-//! reader::{FileReader, SerializedFileReader},
-//! writer::SerializedFileWriter,
-//! },
-//! schema::parser::parse_message_type,
-//! };
+//! # use std::{fs, path::Path, sync::Arc};
+//! #
+//! # use parquet::{
+//! # column::{reader::ColumnReader, writer::ColumnWriter},
+//! # data_type::Int32Type,
+//! # file::{
+//! # properties::WriterProperties,
+//! # reader::{FileReader, SerializedFileReader},
+//! # writer::SerializedFileWriter,
+//! # },
+//! # schema::parser::parse_message_type,
Review Comment:
drive by doc cleanup?
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
The loss of PartialEq / Eq seems like it may cause a fairly major API breakage (if a ParquetError is embedded in another error type, for example)
Could we possibly add in a `PartialEq` impl that returns `None` for comparing `External` variants?
##########
parquet/src/errors.rs:
##########
@@ -39,66 +40,72 @@ pub enum ParquetError {
/// Returned when reading into arrow or writing from arrow.
ArrowError(String),
IndexOutOfBound(usize, usize),
+ /// An external error variant
+ External(Box<dyn Error + Send + Sync>),
}
impl std::fmt::Display for ParquetError {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
- match *self {
- ParquetError::General(ref message) => {
+ match &self {
+ ParquetError::General(message) => {
write!(fmt, "Parquet error: {}", message)
}
- ParquetError::NYI(ref message) => write!(fmt, "NYI: {}", message),
- ParquetError::EOF(ref message) => write!(fmt, "EOF: {}", message),
+ ParquetError::NYI(message) => write!(fmt, "NYI: {}", message),
+ ParquetError::EOF(message) => write!(fmt, "EOF: {}", message),
#[cfg(feature = "arrow")]
- ParquetError::ArrowError(ref message) => write!(fmt, "Arrow: {}", message),
- ParquetError::IndexOutOfBound(ref index, ref bound) => {
+ ParquetError::ArrowError(message) => write!(fmt, "Arrow: {}", message),
+ ParquetError::IndexOutOfBound(index, ref bound) => {
write!(fmt, "Index {} out of bound: {}", index, bound)
}
+ ParquetError::External(e) => write!(fmt, "External: {}", e),
}
}
}
-impl std::error::Error for ParquetError {
- fn cause(&self) -> Option<&dyn ::std::error::Error> {
- None
+impl Error for ParquetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
+ match self {
+ ParquetError::External(e) => Some(e.as_ref()),
+ _ => None,
+ }
}
}
impl From<io::Error> for ParquetError {
fn from(e: io::Error) -> ParquetError {
- ParquetError::General(format!("underlying IO error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
#[cfg(any(feature = "snap", test))]
impl From<snap::Error> for ParquetError {
fn from(e: snap::Error) -> ParquetError {
- ParquetError::General(format!("underlying snap error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
impl From<thrift::Error> for ParquetError {
fn from(e: thrift::Error) -> ParquetError {
- ParquetError::General(format!("underlying Thrift error: {}", e))
+ ParquetError::External(Box::new(e))
}
}
impl From<cell::BorrowMutError> for ParquetError {
fn from(e: cell::BorrowMutError) -> ParquetError {
- ParquetError::General(format!("underlying borrow error: {}", e))
+ ParquetError::External(Box::new(e))
Review Comment:
nice
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
I don't have a great idea for Copy other than to add a `Copy` bound to External
##########
parquet/src/errors.rs:
##########
@@ -39,66 +40,72 @@ pub enum ParquetError {
/// Returned when reading into arrow or writing from arrow.
ArrowError(String),
IndexOutOfBound(usize, usize),
+ /// An external error variant
+ External(Box<dyn Error + Send + Sync>),
}
impl std::fmt::Display for ParquetError {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
- match *self {
- ParquetError::General(ref message) => {
+ match &self {
+ ParquetError::General(message) => {
write!(fmt, "Parquet error: {}", message)
}
- ParquetError::NYI(ref message) => write!(fmt, "NYI: {}", message),
- ParquetError::EOF(ref message) => write!(fmt, "EOF: {}", message),
+ ParquetError::NYI(message) => write!(fmt, "NYI: {}", message),
+ ParquetError::EOF(message) => write!(fmt, "EOF: {}", message),
#[cfg(feature = "arrow")]
- ParquetError::ArrowError(ref message) => write!(fmt, "Arrow: {}", message),
- ParquetError::IndexOutOfBound(ref index, ref bound) => {
+ ParquetError::ArrowError(message) => write!(fmt, "Arrow: {}", message),
+ ParquetError::IndexOutOfBound(index, ref bound) => {
write!(fmt, "Index {} out of bound: {}", index, bound)
}
+ ParquetError::External(e) => write!(fmt, "External: {}", e),
}
}
}
-impl std::error::Error for ParquetError {
- fn cause(&self) -> Option<&dyn ::std::error::Error> {
- None
+impl Error for ParquetError {
+ fn source(&self) -> Option<&(dyn Error + 'static)> {
Review Comment:
❤️
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082903400
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
> I worry that lulls people into a false sense of security,
What is "false" about the sense of security 😆
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
> I worry that lulls people into a false sense of security,
What is "false" about the sense of security 😆 ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3574: Add external variant to ParquetError (#3285)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3574:
URL: https://github.com/apache/arrow-rs/pull/3574#discussion_r1082903996
##########
parquet/src/errors.rs:
##########
@@ -17,12 +17,13 @@
//! Common Parquet errors and macros.
+use std::error::Error;
use std::{cell, io, result, str};
#[cfg(feature = "arrow")]
use arrow_schema::ArrowError;
-#[derive(Debug, PartialEq, Clone, Eq)]
Review Comment:
That we don't actually provide a meaningful PartialEq implementation for a large portion of the errors we produce
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org