-
Notifications
You must be signed in to change notification settings - Fork 95
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
Support for Union-Types (Serde, Schemas, Avro-Values) #76
Conversation
894f1bb
to
12f76e5
Compare
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.
Looks very good overall, just a comment :)
src/types.rs
Outdated
.find_schema(&v) | ||
.ok_or_else(|| SchemaResolutionError::new("Could not find matching type in union"))?; | ||
v.resolve(inner) | ||
match self { |
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 looks like this code is missing the resolution when the writer value is not a union but reader is a union.
e.g: writer writes a Long
, reader is Union(Null, Long)
, then resolve_union
should output a Union(1, Value::Long)
See the "if reader's is a union, but writer's is not" section in https://avro.apache.org/docs/1.8.2/spec.html#Schema+Resolution
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.
The Standard relies on availability of both writer- and reader-schema for resolution.
So I believe the signature of the method should go as pub(crate) fn resolve(reader_schema, writer_schema) -> Result<Value, Error>
.
The only usage of the method is pub fn from_avro_datum<R: Read>
. I think it only would be right to remove the method value.resolve
from the public API.
…o create a union-schema other than parsing a string
Any movement on this? I have an incoming PR for Schema Compatibility that would gain a bit by having this PR in. |
I would also benefit from having this work merged. Is there anything I can do to help? |
There is also #60 implementing bits and pieces of the same feature in a slightly different way and both PRs are pretty old... (definitely because of the fault of us maintainers). I'll need to budget some time to properly understand how serde handles enums (tonight I only managed to read the serialization half of the data format) and at the same time follow up with the authors to understand who is still interested in contributing. @RGafiyatullin are you still interested in contributing? Shall we try to revitalize this PR? I asked the same thing to @jdeschenes on #60. |
I believe that #110 landed the same feature of this PR only in a slightly different form. Closing for now, but please feel free to open a new one based on current master in case there is still something missing that you'd like to see added. |
Documenting avro-rs v0.14.0 (/home/martin/git/apache/avro/lang/rust) warning: this URL is not a hyperlink --> src/lib.rs:696:5 | 696 | //! flavray/avro-rs#76). | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<https://github.com/flavray/avro-rs/pull/76>` | = note: `#[warn(rustdoc::bare_urls)]` on by default = note: bare URLs are not automatically turned into clickable links
* AVRO-3205 Update Cargo.toml [package] information * AVRO-3205 Fix 'cargo doc` warning Documenting avro-rs v0.14.0 (/home/martin/git/apache/avro/lang/rust) warning: this URL is not a hyperlink --> src/lib.rs:696:5 | 696 | //! flavray/avro-rs#76). | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<https://github.com/flavray/avro-rs/pull/76>` | = note: `#[warn(rustdoc::bare_urls)]` on by default = note: bare URLs are not automatically turned into clickable links * AVRO-3205 Add keywords and documentation to [package] * AVRO-3205 Add categories = ["encoding"] * AVRO-3205 Update markdown files Replace flavray/avro-rs with apache/avro. Replace MIT license with Apache License 2
* AVRO-3205 Update Cargo.toml [package] information * AVRO-3205 Fix 'cargo doc` warning Documenting avro-rs v0.14.0 (/home/martin/git/apache/avro/lang/rust) warning: this URL is not a hyperlink --> src/lib.rs:696:5 | 696 | //! flavray/avro-rs#76). | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<https://github.com/flavray/avro-rs/pull/76>` | = note: `#[warn(rustdoc::bare_urls)]` on by default = note: bare URLs are not automatically turned into clickable links * AVRO-3205 Add keywords and documentation to [package] * AVRO-3205 Add categories = ["encoding"] * AVRO-3205 Update markdown files Replace flavray/avro-rs with apache/avro. Replace MIT license with Apache License 2 (cherry picked from commit ea07ac0)
Remove a link to an issue/PR at flavray/avro-rs/pull/76 because it is already resolved. Enable (uncomment) the tests related to this issue. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Remove a link to an issue/PR at flavray/avro-rs/pull/76 because it is already resolved. Enable (uncomment) the tests related to this issue. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> (cherry picked from commit 69fda12)
* AVRO-3205 Update Cargo.toml [package] information * AVRO-3205 Fix 'cargo doc` warning Documenting avro-rs v0.14.0 (/home/martin/git/apache/avro/lang/rust) warning: this URL is not a hyperlink --> src/lib.rs:696:5 | 696 | //! flavray/avro-rs#76). | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<https://github.com/flavray/avro-rs/pull/76>` | = note: `#[warn(rustdoc::bare_urls)]` on by default = note: bare URLs are not automatically turned into clickable links * AVRO-3205 Add keywords and documentation to [package] * AVRO-3205 Add categories = ["encoding"] * AVRO-3205 Update markdown files Replace flavray/avro-rs with apache/avro. Replace MIT license with Apache License 2
Remove a link to an issue/PR at flavray/avro-rs/pull/76 because it is already resolved. Enable (uncomment) the tests related to this issue. Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Difference between the
Value::Union
representation this PR and @kphelps 's one:Value::Union(variant_index: usize, variant_value: Box<Value>)
instead ofValue::Union(union_ref: UnionRef, variant_value: Box<Value>)
.This option does not allow for creation of union-type instance from the inner variant-value "implicitly" (i.e. without knowing the variant-index able to carry that exact variant-data).
This should help a lot with Serde because this restriction allows "lossless" translations between Structs<->Avro-Values<->Avro-Binary in both directions.
Example of usage: https://gist.github.com/RGafiyatullin/4fec518e3cd8e3c6fe718ca976a4cdc5
Output: https://gist.github.com/RGafiyatullin/8aaf4c427bec5456edde2e0913dde472