You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/11/12 01:40:11 UTC
[GitHub] [avro] lulitao1997 opened a new pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
lulitao1997 opened a new pull request #1396:
URL: https://github.com/apache/avro/pull/1396
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
- [x ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO-3248) issues and references them in the PR title.
### Tests
- [ x] My PR adds the following unit tests
1. schema::tests::test_union_of_records
2. schema::tests::test_nullable_record
### Commits
- [x ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g merged pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #1396:
URL: https://github.com/apache/avro/pull/1396
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1396:
URL: https://github.com/apache/avro/pull/1396#issuecomment-1016167191
I start working on this PR!
Hopefully it will be merged later today!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1396:
URL: https://github.com/apache/avro/pull/1396#discussion_r787504710
##########
File path: lang/rust/src/types.rs
##########
@@ -168,7 +173,11 @@ where
T: Into<Self>,
{
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"
Review comment:
Ignore this comment! I misunderstood it the first time!
It looks OK to me!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1396:
URL: https://github.com/apache/avro/pull/1396#discussion_r781173867
##########
File path: lang/rust/src/types.rs
##########
@@ -66,7 +66,12 @@ pub enum Value {
/// reading values.
Enum(i32, String),
/// An `union` Avro value.
- Union(Box<Value>),
+ ///
+ /// A Union is represented by the value it holds and its position in the type list
+ /// of its corresponding schema
+ /// This allows schema-less encoding, as well as schema resolution while
+ /// reading values.
+ Union(i32, Box<Value>),
Review 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.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1396:
URL: https://github.com/apache/avro/pull/1396#discussion_r781171826
##########
File path: lang/rust/src/types.rs
##########
@@ -66,7 +66,12 @@ pub enum Value {
/// reading values.
Enum(i32, String),
/// An `union` Avro value.
- Union(Box<Value>),
+ ///
+ /// A Union is represented by the value it holds and its position in the type list
+ /// of its corresponding schema
+ /// This allows schema-less encoding, as well as schema resolution while
+ /// reading values.
+ Union(i32, Box<Value>),
Review comment:
Why `i32` ? Could we have negative values ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a change in pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1396:
URL: https://github.com/apache/avro/pull/1396#discussion_r787485297
##########
File path: lang/rust/src/types.rs
##########
@@ -703,13 +714,14 @@ impl Value {
fn resolve_union(self, schema: &UnionSchema) -> Result<Self, Error> {
let v = match self {
// Both are unions case.
- Value::Union(v) => *v,
+ Value::Union(_i, v) => *v,
// Reader is a union, but writer is not.
v => v,
};
// 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
Review 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.
##########
File path: lang/rust/src/de.rs
##########
@@ -162,7 +162,7 @@ impl<'de> de::EnumAccess<'de> for EnumDeserializer<'de> {
self.input.first().map_or(
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((
Review comment:
This change does not look related to this PR.
@lulitao1997 Why did you make it ?
##########
File path: lang/rust/src/types.rs
##########
@@ -168,7 +173,11 @@ where
T: Into<Self>,
{
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"
Review comment:
This seems incorrect also when the union has more than two items.
And looks like a stopper for this PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] praetp commented on pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
praetp commented on pull request #1396:
URL: https://github.com/apache/avro/pull/1396#issuecomment-1015832184
I am interested in this change.
Didn't compile for me. Maybe I did something wrong ?
```
error[E0061]: this enum variant takes 2 arguments but 1 argument was supplied
--> /home/spetsnaz/.cargo/git/checkouts/avro-d44366ca41b07ddf/912e979/lang/rust/src/decode.rs:209:20
|
209 | Ok(Value::Union(Box::new(value)))
| ^^^^^^^^^^^^ --------------- supplied 1 argument
| |
| expected 2 arguments
|
note: tuple variant defined here
--> /home/spetsnaz/.cargo/git/checkouts/avro-d44366ca41b07ddf/912e979/lang/rust/src/types.rs:74:5
|
74 | Union(i32, Box<Value>),
| ^^^^^
```
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1396:
URL: https://github.com/apache/avro/pull/1396#issuecomment-1016193760
@praetp Please try the PR now! It is fixed and rebased to latest master!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1396: AVRO-3248: Rust: Support named types in UnionSchema
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #1396:
URL: https://github.com/apache/avro/pull/1396#issuecomment-1016390372
Thank you for the contribution, @lulitao1997 !
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org