You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "sarutak (via GitHub)" <gi...@apache.org> on 2023/07/30 07:54:41 UTC
[GitHub] [avro] sarutak opened a new pull request, #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
sarutak opened a new pull request, #2405:
URL: https://github.com/apache/avro/pull/2405
AVRO-3818
## What is the purpose of the change
This PR fixes an issue that with the current Rust binding, inner named types don't inherit their enclosing namespace even though they have no their own namespace.
Given we have a schema like as follows.
```
{
"namespace": "my_ns",
"type": "record",
"aliases": ["my_alias"],
"name": "my_schema",
"fields": [
{
"name": "f1",
"type": {
"name": "enum1",
"type": "enum",
"symbols": ["a"]
}
}
]
}
```
In the canonical form of this schema, the fullname of `enum1` should be `my_ns.enum1` because it has no their own namespace and the enclosing namespace is `my_ns`.
```
{"name":"my_ns.my_schema","type":"record","fields":[{"name":"f1","type":{"name":"my_ns.enum1","type":"enum","symbols":["a"]}}]}
```
But the current implementation makes the fullname of `enum1` as `enum1`.
```
{"name":"my_ns.my_schema","type":"record","fields":[{"name":"f1","type":{"name":"enum1","type":"enum","symbols":["a"]}}]}
```
## Verifying this change
Added new tests and modified existing tests which was wrong.
These tests passed with `cargo test --tests`.
## Documentation
No new features added.
--
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] sarutak commented on a diff in pull request #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #2405:
URL: https://github.com/apache/avro/pull/2405#discussion_r1279645953
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1518,7 +1528,7 @@ impl Parser {
}
let schema = Schema::Enum(EnumSchema {
- name,
+ name: fully_qualified_name.clone(),
Review Comment:
It's not enough because [this test](https://github.com/apache/avro/pull/2405/files#diff-b702ef60e00210dede234ba524d4ce1d4ff14ef199191ec46a7f4ba8a6bd211eR4944) doesn't pass.
--
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 #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2405:
URL: https://github.com/apache/avro/pull/2405#issuecomment-1658995416
Thank you for the contribution, @sarutak !
--
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 merged pull request #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #2405:
URL: https://github.com/apache/avro/pull/2405
--
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 diff in pull request #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2405:
URL: https://github.com/apache/avro/pull/2405#discussion_r1278974808
##########
lang/rust/avro/src/schema.rs:
##########
@@ -4865,4 +4875,155 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_avro_3818_inherit_enclosing_namespace() -> TestResult {
+ // Enclosing namespace is specified but innner namespaces are not.
+ let schema_str = r#"
+ {
+ "namespace": "my_ns",
+ "type": "record",
+ "name": "my_schema",
+ "fields": [
+ {
+ "name": "f1",
+ "type": {
+ "name": "enum1",
+ "type": "enum",
+ "symbols": ["a"]
+ }
+ }, {
+ "name": "f2",
+ "type": {
+ "name": "fixed1",
+ "type": "fixed",
+ "size": 1
+ }
+ }
+ ]
+ }
+ "#;
+
+ let expected = r#"{"name":"my_ns.my_schema","type":"record","fields":[{"name":"f1","type":{"name":"my_ns.enum1","type":"enum","symbols":["a"]}},{"name":"f2","type":{"name":"my_ns.fixed1","type":"fixed","size":1}}]}"#;
+ let schema = Schema::parse_str(schema_str)?;
+ let canonical_form = schema.canonical_form();
+ assert_eq!(canonical_form, expected);
+
+ // Enclosing namespace and inner namespaces are specified
+ // but innner namespaces are ""
Review Comment:
```suggestion
// but inner namespaces are ""
```
##########
lang/rust/avro/src/schema.rs:
##########
@@ -4865,4 +4875,155 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_avro_3818_inherit_enclosing_namespace() -> TestResult {
+ // Enclosing namespace is specified but innner namespaces are not.
Review Comment:
```suggestion
// Enclosing namespace is specified but inner namespaces are not.
```
--
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] clesaec commented on a diff in pull request #2405: AVRO-3818: [Rust] Let inner named types inherit their enclosing namespace if they have no their own namespace
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2405:
URL: https://github.com/apache/avro/pull/2405#discussion_r1278974077
##########
lang/rust/avro/src/schema.rs:
##########
@@ -1518,7 +1528,7 @@ impl Parser {
}
let schema = Schema::Enum(EnumSchema {
- name,
+ name: fully_qualified_name.clone(),
Review Comment:
Wouldn't it have been sufficient to ; keeping
`let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);`
without modifying Name::parse method ?
--
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