You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "martin-g (via GitHub)" <gi...@apache.org> on 2023/08/15 07:20:12 UTC
[GitHub] [avro] martin-g opened a new pull request, #2441: AVRO-3814: Fix schema resolution for records in union types
martin-g opened a new pull request, #2441:
URL: https://github.com/apache/avro/pull/2441
AVRO-3814
Successor-of: https://github.com/apache/avro/pull/2394
## What is the purpose of the change
* Use `types::Value::resolve_internal()` to find the matching schema in an union
## Documentation
- Does this pull request introduce a new feature? no
--
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 #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2441:
URL: https://github.com/apache/avro/pull/2441#discussion_r1294451602
##########
lang/rust/avro/src/schema.rs:
##########
@@ -837,9 +837,11 @@ impl UnionSchema {
// extend known schemas with just resolved names
collected_names.extend(resolved_names);
let namespace = &schema.namespace().or_else(|| enclosing_namespace.clone());
+
value
- .validate_internal(schema, &collected_names, namespace)
- .is_none()
+ .clone()
Review Comment:
This is needed because `#resolve_internal()` accepts `mut self` ... I'll see whether this could be optimized somehow!
--
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] RikHeijdens commented on a diff in pull request #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "RikHeijdens (via GitHub)" <gi...@apache.org>.
RikHeijdens commented on code in PR #2441:
URL: https://github.com/apache/avro/pull/2441#discussion_r1294462122
##########
lang/rust/avro/src/schema.rs:
##########
@@ -837,9 +837,11 @@ impl UnionSchema {
// extend known schemas with just resolved names
collected_names.extend(resolved_names);
let namespace = &schema.namespace().or_else(|| enclosing_namespace.clone());
+
value
- .validate_internal(schema, &collected_names, namespace)
- .is_none()
+ .clone()
Review Comment:
Yep - I understand the rationale. I had contemplated this exact solution and went with the other one because it avoided an allocation :-)
--
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 #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #2441:
URL: https://github.com/apache/avro/pull/2441
--
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] RikHeijdens commented on a diff in pull request #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "RikHeijdens (via GitHub)" <gi...@apache.org>.
RikHeijdens commented on code in PR #2441:
URL: https://github.com/apache/avro/pull/2441#discussion_r1294317781
##########
lang/rust/avro/src/types.rs:
##########
@@ -377,6 +377,10 @@ impl Value {
}
}
+ /// Validates the value against the provided schema.
+ ///
+ /// Arguments:
+ /// * `schema_resolution` - whether schema resolution rules should be applied when validating the `value`.
Review Comment:
Please remove the `Arguments` section from the doc comment or update the description.
##########
lang/rust/avro/src/schema.rs:
##########
@@ -837,9 +837,11 @@ impl UnionSchema {
// extend known schemas with just resolved names
collected_names.extend(resolved_names);
let namespace = &schema.namespace().or_else(|| enclosing_namespace.clone());
+
value
- .validate_internal(schema, &collected_names, namespace)
- .is_none()
+ .clone()
Review Comment:
In contrast to my previously proposed solution this may cause the entire `value` to be copied, just for the purpose of validation. That seems rather expensive - but logically this would work.
--
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 #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2441:
URL: https://github.com/apache/avro/pull/2441#discussion_r1294466426
##########
lang/rust/avro/src/schema.rs:
##########
@@ -837,9 +837,11 @@ impl UnionSchema {
// extend known schemas with just resolved names
collected_names.extend(resolved_names);
let namespace = &schema.namespace().or_else(|| enclosing_namespace.clone());
+
value
- .validate_internal(schema, &collected_names, namespace)
- .is_none()
+ .clone()
Review Comment:
I tried to optimize it but without success.
We could try harder if someone complains that this is an actual performance issue!
--
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 #2441: AVRO-3814: Fix schema resolution for records in union types
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2441:
URL: https://github.com/apache/avro/pull/2441#discussion_r1294465570
##########
lang/rust/avro/src/types.rs:
##########
@@ -377,6 +377,10 @@ impl Value {
}
}
+ /// Validates the value against the provided schema.
+ ///
+ /// Arguments:
+ /// * `schema_resolution` - whether schema resolution rules should be applied when validating the `value`.
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