AVRO-3248: Rust: Support named types in UnionSchema#1396
Merged
martin-g merged 9 commits intoapache:masterfrom Jan 19, 2022
Merged
AVRO-3248: Rust: Support named types in UnionSchema#1396martin-g merged 9 commits intoapache:masterfrom
martin-g merged 9 commits intoapache:masterfrom
Conversation
3 tasks
martin-g
reviewed
Jan 10, 2022
| /// of its corresponding schema | ||
| /// This allows schema-less encoding, as well as schema resolution while | ||
| /// reading values. | ||
| Union(i32, Box<Value>), |
Member
There was a problem hiding this comment.
Why i32 ? Could we have negative values ?
Member
There was a problem hiding this comment.
I see, it just follows the way Value::Enum is implemented.
I'll take a deeper look why i32 is used there. It also leads to a lot of casting later.
|
I am interested in this change. Didn't compile for me. Maybe I did something wrong ? |
Member
|
I start working on this PR! |
previously union does not support named types, and we will get error if we add 2 records into 1 UnionSchema.
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
force-pushed
the
record_union
branch
from
717e114 to
f82e1e7
Compare
Member
|
@praetp Please try the PR now! It is fixed and rebased to latest master! |
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
martin-g
reviewed
Jan 19, 2022
| Err(de::Error::custom("A record must have a least one field")), | ||
| |item| match (item.0.as_ref(), &item.1) { | ||
| ("type", Value::String(x)) => Ok(( | ||
| ("type", Value::String(x)) | ("type", Value::Enum(_, x)) => Ok(( |
Member
There was a problem hiding this comment.
This change does not look related to this PR.
@lulitao1997 Why did you make it ?
lang/rust/src/types.rs
Outdated
| // Find the first match in the reader schema. | ||
| let (_, inner) = schema.find_schema(&v).ok_or(Error::FindUnionVariant)?; | ||
| Ok(Value::Union(Box::new(v.resolve(inner)?))) | ||
| // FIXME: this is wrong when the union consists of multiple same records that have different names |
Member
There was a problem hiding this comment.
True, but I wonder how big a problem it is ?
The name and doc might be wrong but the fields will be the same, so the application logic should work.
lang/rust/src/types.rs
Outdated
| { | ||
| fn from(value: Option<T>) -> Self { | ||
| Self::Union(Box::new(value.map_or_else(|| Self::Null, Into::into))) | ||
| // NOTE: this is incorrect in case first type in union is not "none" |
Member
There was a problem hiding this comment.
This seems incorrect also when the union has more than two items.
And looks like a stopper for this PR.
Member
There was a problem hiding this comment.
Ignore this comment! I misunderstood it the first time!
It looks OK to me!
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Member
|
Thank you for the contribution, @lulitao1997 ! |
martin-g
added a commit
that referenced
this pull request
Feb 1, 2022
* AVRO-3248: Rust: Support named types in UnionSchema previously union does not support named types, and we will get error if we add 2 records into 1 UnionSchema. * AVRO-3248: Fix a typo in error message * AVRO-3248: Give better names for the schemata as string * AVRO-3248: More better names for variables * AVRO-3248: Code formatting * AVRO-3248 Fix formatting & build Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * AVRO-3248: Fix generate_interop_data and formatting Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * AVRO-3248: Fix some regressions after the rebase to master Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * AVRO-3248 Fix the position in the Union for the Double value Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com> Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> (cherry picked from commit 49c6e10)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
change Value::Union to value it holds and its position in the type list,
this allows us to
Make sure you have checked all steps below.
Jira
Tests
Commits