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 2022/03/06 14:29:47 UTC
[GitHub] [avro] martin-g opened a new pull request #1580: AVRO-3433: Rust: The canonical form should preserve schema references
martin-g opened a new pull request #1580:
URL: https://github.com/apache/avro/pull/1580
Do not resolve Schema::Ref when printing as JSON.
The Java SDK complains that a type is redefined if it is not a Ref the
second time
### Jira
- [X] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3433
-
### Tests
- [X] My PR adds and updates some unit tests
### 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"
### Documentation
- [X] No need of new documentation
--
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 #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #1580:
URL: https://github.com/apache/avro/pull/1580
--
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 a change in pull request #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1580:
URL: https://github.com/apache/avro/pull/1580#discussion_r820397299
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -241,17 +242,41 @@ impl Name {
}
}
+ /// Create a new `Name`.
+ /// Parses the optional `namespace` from the `name` string.
+ /// `aliases` will not be defined.
+ pub fn parse_str(name: &str) -> Name {
Review comment:
Done!
--
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] jklamer commented on a change in pull request #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
jklamer commented on a change in pull request #1580:
URL: https://github.com/apache/avro/pull/1580#discussion_r820299079
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -241,17 +242,41 @@ impl Name {
}
}
+ /// Create a new `Name`.
+ /// Parses the optional `namespace` from the `name` string.
+ /// `aliases` will not be defined.
+ pub fn parse_str(name: &str) -> Name {
Review comment:
If we are worried about breaking backwards compatibility for those who were using "namespace.name", might be work deprecating "new" in favor of this? Or an implementation of `From<&str>` for Name?
--
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 #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1580:
URL: https://github.com/apache/avro/pull/1580#discussion_r820393107
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -241,17 +242,41 @@ impl Name {
}
}
+ /// Create a new `Name`.
+ /// Parses the optional `namespace` from the `name` string.
+ /// `aliases` will not be defined.
+ pub fn parse_str(name: &str) -> Name {
Review comment:
I also wondered whether to modify `new()` but its documentation explicitly says that it doesn't parse and set the namespace.
Since you also noticed it I think we should just do it, because currently `new()` is broken!
--
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 #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #1580:
URL: https://github.com/apache/avro/pull/1580#discussion_r820391478
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -292,6 +317,20 @@ impl Name {
}
}
+impl Hash for Name {
Review comment:
According to the [spec](https://avro.apache.org/docs/current/spec.html#names) the `Equality of names is defined on the fullname.`. If we used derivation it will take into account also the optional aliases.
--
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] jklamer commented on a change in pull request #1580: AVRO-3433: Rust: The canonical form should preserve schema references
Posted by GitBox <gi...@apache.org>.
jklamer commented on a change in pull request #1580:
URL: https://github.com/apache/avro/pull/1580#discussion_r820296389
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -435,15 +474,15 @@ fn parse_json_integer_for_decimal(value: &serde_json::Number) -> Result<DecimalM
#[derive(Default)]
struct Parser {
- input_schemas: HashMap<String, Value>,
+ input_schemas: HashMap<Name, Value>,
Review comment:
This makes me happy
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -292,6 +317,20 @@ impl Name {
}
}
+impl Hash for Name {
Review comment:
Is there a reason to not use derive to implement Hash?
##########
File path: lang/rust/avro/src/schema.rs
##########
@@ -241,17 +242,41 @@ impl Name {
}
}
+ /// Create a new `Name`.
+ /// Parses the optional `namespace` from the `name` string.
+ /// `aliases` will not be defined.
+ pub fn parse_str(name: &str) -> Name {
Review comment:
I think It would make sense to run this on the input to `Name::new`. I think it would make the SDK more consistent. I would be worried by confusing of `Name::new(&schema_str[..]) != Name::parse_str(&schema_str[..])`
--
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