-
Notifications
You must be signed in to change notification settings - Fork 14
Adds error serialization-deserialization #69
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.
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
Conversation
c9bec71 to
b2b1434
Compare
…n_error functions
…he Arrow Flight reader
b2b1434 to
0f8e7c5
Compare
NGA-TRAN
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice infrastructure, Gabriel. You make things very easy for us to send and use error
| pub struct ArrowErrorProto { | ||
| #[prost(string, optional, tag = "1")] | ||
| pub ctx: Option<String>, | ||
| #[prost( | ||
| oneof = "ArrowErrorInnerProto", | ||
| tags = "2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19" | ||
| )] | ||
| pub inner: Option<ArrowErrorInnerProto>, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: If a new error number 20 is added in the future, can we simply add 20 here without worrying about backward compatibility?
Also this means whenever we upgrade datafusion and there are new arrow (datafusion, parquet, ...) errors, we have to add them here (or in the right error file) right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. The only thing we cannot do is to tweak the order of the messages, or deleting numbers, but adding new numbers (and therefore error variants), is backwards compatible.
If there's a DataFusion upgrade and a new error variant is added, we'll need to come here and add it ourselves, otherwise the Rust compiler will tell us there are some error variants not handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be perfect if we get compile error
| Collection(DataFusionCollectionErrorProto), | ||
| #[prost(message, boxed, tag = "19")] | ||
| Shared(Box<DataFusionErrorProto>), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is very nice that you make and put all Arrow, Parquet, Schema, Context, SQL parser,... error protos in here so we only worry about serialize and deserialize one type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was quite a bit of work, but it's something that's probably going to need close 0 maintenance, and it's going to help us a lot in knowing what goes wrong.
I expect initially to see more errors than actual queries running.
| assert_eq!( | ||
| DataFusionError::Execution("something failed".to_string()).to_string(), | ||
| err.to_string() | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am amazed reading this test. We do not have to convert anything here. You already deserialize and covert the error in arrow_flight_read.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I expect these integration tests to be key not only for ensuring our changes remain healthy, but also for development and debugging.
If they feel like magic it means that we are on the right track.
robtandy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me and will allow for propagation of root cause errors; thank you @gabotechs !
Allows propagating
DataFusionErrors over the wire so that upstream stages can read the original error without loosing information.This is done by providing some mirroring protobuf messages for all the current variants of
DataFusionErrors. As these errors might also hold references to other errors from other libraries, also those other error message get a mirroring protobuf message.This PR ships a bunch of boilerplate code, but the actual important parts and the only ones that are exposed to the rest of the project are just two functions:
Everything else is hidden in the
src/errors/modulesThe PR ships both unit tests for all the error variations and an integration tests that checks that errors are properly propagated through the networks and can be recovered on the receiving end