You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "ftelnov (via GitHub)" <gi...@apache.org> on 2023/06/01 22:16:02 UTC
[GitHub] [avro] ftelnov opened a new pull request, #2266: Rust: fix complex Ref resolving in Union
ftelnov opened a new pull request, #2266:
URL: https://github.com/apache/avro/pull/2266
(no comment)
--
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] ftelnov commented on pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2266:
URL: https://github.com/apache/avro/pull/2266#issuecomment-1573406418
I also allowed myself to do a slight renaming and signature changes in ResolvedSchema - as previously it was a bit inconvenient to use and read
--
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] ftelnov commented on a diff in pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on code in PR #2266:
URL: https://github.com/apache/avro/pull/2266#discussion_r1219634813
##########
lang/rust/avro/src/schema.rs:
##########
@@ -434,21 +455,28 @@ impl<'s> ResolvedSchema<'s> {
}
Schema::Record(RecordSchema { name, fields, .. }) => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref
+ if self
+ .names_ref
.insert(fully_qualified_name.clone(), schema)
.is_some()
{
return Err(Error::AmbiguousSchemaDefinition(fully_qualified_name));
} else {
let record_namespace = fully_qualified_name.namespace;
for field in fields {
- Self::from_internal(vec![&field.schema], names_ref, &record_namespace)?
+ self.resolve(vec![&field.schema], &record_namespace, known_schemas)?
}
}
}
Schema::Ref { name } => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref.get(&fully_qualified_name).is_none() {
+ // first search for reference in current schema, then look into external references.
+ let is_resolved_locally = self.names_ref.get(&fully_qualified_name).is_some();
+ let is_resolved_with_known_schemas = known_schemas
+ .as_ref()
+ .map(|schemas| schemas.get(&fully_qualified_name).is_some())
+ .unwrap_or(false);
+ if !is_resolved_locally && !is_resolved_with_known_schemas {
Review Comment:
I hope compiler would solve such a thing for us - mostly I wrote it to enhance readability. But yeah, you are right, we'd better state it explicitly
--
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 #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2266:
URL: https://github.com/apache/avro/pull/2266#issuecomment-1583244529
Thank you, @ftelnov !
--
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 diff in pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2266:
URL: https://github.com/apache/avro/pull/2266#discussion_r1219419592
##########
lang/rust/avro/src/schema.rs:
##########
@@ -434,21 +455,28 @@ impl<'s> ResolvedSchema<'s> {
}
Schema::Record(RecordSchema { name, fields, .. }) => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref
+ if self
+ .names_ref
.insert(fully_qualified_name.clone(), schema)
.is_some()
{
return Err(Error::AmbiguousSchemaDefinition(fully_qualified_name));
} else {
let record_namespace = fully_qualified_name.namespace;
for field in fields {
- Self::from_internal(vec![&field.schema], names_ref, &record_namespace)?
+ self.resolve(vec![&field.schema], &record_namespace, known_schemas)?
}
}
}
Schema::Ref { name } => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref.get(&fully_qualified_name).is_none() {
+ // first search for reference in current schema, then look into external references.
+ let is_resolved_locally = self.names_ref.get(&fully_qualified_name).is_some();
+ let is_resolved_with_known_schemas = known_schemas
+ .as_ref()
+ .map(|schemas| schemas.get(&fully_qualified_name).is_some())
+ .unwrap_or(false);
+ if !is_resolved_locally && !is_resolved_with_known_schemas {
Review Comment:
I have the feeling this is not what you wanted.
IMO it should be:
```
if !is_resolved_locally {
let is_resolved_with_known_schemas = known_schemas
.as_ref()
.map(|schemas| schemas.get(&fully_qualified_name).is_some())
.unwrap_or(false);
if !is_resolved_with_known_schemas {
return Err(Error::SchemaResolutionError(fully_qualified_name));
}
}
```
I am already doing some improvements to the PR, so please just comment here, don't push changes,
--
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] ftelnov commented on pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2266:
URL: https://github.com/apache/avro/pull/2266#issuecomment-1573371685
Now the fix is ready - might contain some dirty code, please review patiently.
What was the issue: at the resolve_union method, we prematurely resolved all references in the Union. It was done in the wrong way - it was resolving only top-level schemas, without recursive lookup. I think it is bad solution even if it might work after fix - firstly, it adds complexity, secondly it is inconsistent with union validation, which does not do premature Ref resolving.
I suppose, it was done that way because `ResolvedSchema` does not handle external references. I've tried to fix it - I added a possibility to resolve external references to `ResolvedSchema`(with an extra argument and new constructor). And I adjusted `find_schema` method of the union to include external refs handle.
--
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 diff in pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2266:
URL: https://github.com/apache/avro/pull/2266#discussion_r1221449912
##########
lang/rust/avro/src/schema.rs:
##########
@@ -434,21 +455,28 @@ impl<'s> ResolvedSchema<'s> {
}
Schema::Record(RecordSchema { name, fields, .. }) => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref
+ if self
+ .names_ref
.insert(fully_qualified_name.clone(), schema)
.is_some()
{
return Err(Error::AmbiguousSchemaDefinition(fully_qualified_name));
} else {
let record_namespace = fully_qualified_name.namespace;
for field in fields {
- Self::from_internal(vec![&field.schema], names_ref, &record_namespace)?
+ self.resolve(vec![&field.schema], &record_namespace, known_schemas)?
}
}
}
Schema::Ref { name } => {
let fully_qualified_name = name.fully_qualified_name(enclosing_namespace);
- if names_ref.get(&fully_qualified_name).is_none() {
+ // first search for reference in current schema, then look into external references.
+ let is_resolved_locally = self.names_ref.get(&fully_qualified_name).is_some();
+ let is_resolved_with_known_schemas = known_schemas
+ .as_ref()
+ .map(|schemas| schemas.get(&fully_qualified_name).is_some())
+ .unwrap_or(false);
+ if !is_resolved_locally && !is_resolved_with_known_schemas {
Review Comment:
My point was that we should do it lazily:
- first check in the local ones
- if not resolved then look in the known ones
- if again not resolved then return an Error
There is no need to look in the known ones if it could be resolved in the local ones.
--
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 #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2266:
URL: https://github.com/apache/avro/pull/2266#issuecomment-1580639980
Please review https://github.com/apache/avro/pull/2266/commits/59cb2b2ca6149c102fa08bc112dc87d259248baa and let me know if I broke something!
--
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] ftelnov commented on pull request #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2266:
URL: https://github.com/apache/avro/pull/2266#issuecomment-1583062134
@martin-g everything seems fine to me, despite that naming I've mentioned earlier. Good job, thanks for prettifying it!
--
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 #2266: Avro-3767 [Rust] fix complex Ref resolving in Union
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #2266:
URL: https://github.com/apache/avro/pull/2266
--
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