You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "marcosschroh (via GitHub)" <gi...@apache.org> on 2023/11/16 10:37:03 UTC
[PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
marcosschroh opened a new pull request, #2587:
URL: https://github.com/apache/avro/pull/2587
## What is the purpose of the change
Do not panic when calculating schemas compatibilities. The return type is a `Result` which in case of an `error`the user can get feedback about the compatibility issue, otherwise the compatibility will be `Ok()`. Related to [AVRO-3904](https://issues.apache.org/jira/browse/AVRO-3904)
## Verifying this change
This change is partially covered by existing tests, such as `test_union_resolution_no_structure_match, test_missing_field`. WIth the changes I am adding a new test to check the compatibility error and all the existing test will do the same in case of `Err`
## Documentation
- Does this pull request introduce a new feature? (no). The `SchemaCompatibility::can_read` return type has been changed from `bool` to `Result` so this should be in the documentation as well.
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec merged PR #2587:
URL: https://github.com/apache/avro/pull/2587
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1396095597
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -96,10 +109,14 @@ impl Checker {
symbols: r_symbols, ..
}) = readers_schema
{
- return !w_symbols.iter().any(|e| !r_symbols.contains(e));
+ if !w_symbols.iter().any(|e| !r_symbols.contains(e)) {
Review Comment:
Yes, all the symbols in the writer should be in the reader, I think you suggestion makes sense and it is way better.
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395612123
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -133,38 +156,64 @@ impl Checker {
{
for field in r_fields.iter() {
if let Some(pos) = w_lookup.get(&field.name) {
- if !self.full_match_schemas(&w_fields[*pos].schema, &field.schema) {
- return false;
+ if self
+ .full_match_schemas(&w_fields[*pos].schema, &field.schema)
+ .is_err()
+ {
+ return Err(Error::CompatibilityError(format!("Field {} in reader schema does not match the type in the writer schema", field.name)));
}
} else if field.default.is_none() {
- return false;
+ return Err(Error::CompatibilityError(format!(
+ "Field {} in reader schema must have a default value",
+ field.name
+ )));
}
}
}
}
- true
+ Ok(())
}
- fn match_union_schemas(&mut self, writers_schema: &Schema, readers_schema: &Schema) -> bool {
+ fn match_union_schemas(
+ &mut self,
+ writers_schema: &Schema,
+ readers_schema: &Schema,
+ ) -> AvroResult<()> {
let w_type = SchemaKind::from(writers_schema);
let r_type = SchemaKind::from(readers_schema);
assert_eq!(r_type, SchemaKind::Union);
if w_type == SchemaKind::Union {
Review Comment:
maybe you could do this instead, to reduce the nesting
```rs
if w_type != SchemaKind::Union {
return Err(Error::CompatibilityError(String::from(
"readers_schema should have been Schema::Union",
)));
}```
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1396856210
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Currently, AvroResult is defined as
```rust
pub type AvroResult<T> = Result<T, Error>;
```
with Error as this Enum, so you can't derive.
Would be possible with
```rust
#[derive(thiserror::Error, Debug)]
pub enum CompatibilityError {
```
But, then, you could not build an AvroResult (until AvroResult is redefined as `Result<T, thiserror::Error>`, but i don't know what would be other consequences
WDYT @martin-g ?
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1401427611
##########
lang/rust/avro/src/error.rs:
##########
@@ -480,6 +480,39 @@ pub enum Error {
BadCodecMetadata,
}
+#[derive(thiserror::Error, Debug)]
+pub enum CompatibilityError {
+ #[error("Schemas are not compatible. Writer schema is {writer_schema_type}, but reader schema is {reader_schema_type}")]
+ WrongType { writer_schema_type: String, reader_schema_type: String},
+
+ #[error("Schemas are not compatible. The {schema_type} should have been {expected_type}")]
+ TypeExpected {schema_type: String, expected_type: String},
+
+ #[error("Schemas are not compatible. Field '{0}' in reader schema does not match the type in the writer schema")]
+ FieldTypeMissmatch(String),
+
+ #[error("Schemas are not compatible. Schemas missmatch")]
+ SchemaMissmatch,
Review Comment:
Should be ["mismatch"](https://en.wiktionary.org/wiki/mismatch) with one "s".
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on PR #2587:
URL: https://github.com/apache/avro/pull/2587#issuecomment-1820600690
@martin-g Could you take a look and approve the workflows so I can see whether there are any errors? Thanks
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1396142923
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -133,38 +156,64 @@ impl Checker {
{
for field in r_fields.iter() {
if let Some(pos) = w_lookup.get(&field.name) {
- if !self.full_match_schemas(&w_fields[*pos].schema, &field.schema) {
- return false;
+ if self
+ .full_match_schemas(&w_fields[*pos].schema, &field.schema)
+ .is_err()
+ {
+ return Err(Error::CompatibilityError(format!("Field {} in reader schema does not match the type in the writer schema", field.name)));
}
} else if field.default.is_none() {
- return false;
+ return Err(Error::CompatibilityError(format!(
+ "Field {} in reader schema must have a default value",
+ field.name
+ )));
}
}
}
}
- true
+ Ok(())
}
- fn match_union_schemas(&mut self, writers_schema: &Schema, readers_schema: &Schema) -> bool {
+ fn match_union_schemas(
+ &mut self,
+ writers_schema: &Schema,
+ readers_schema: &Schema,
+ ) -> AvroResult<()> {
let w_type = SchemaKind::from(writers_schema);
let r_type = SchemaKind::from(readers_schema);
assert_eq!(r_type, SchemaKind::Union);
if w_type == SchemaKind::Union {
Review Comment:
I solved in a different way but this helped to see that some code was never called and the check `assert_eq!(r_type, SchemaKind::Union);` is actually not needed.
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395654623
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -246,80 +308,144 @@ impl SchemaCompatibility {
attributes: _,
}) = readers_schema
{
- return w_name.name == r_name.name && w_size == r_size;
+ if w_name.name == r_name.name && w_size == r_size {
Review Comment:
You can use here something like:
```rs
(w_name.name == r_name.name && w_size == r_size).then(|| ()).ok_or(Error::CompatibilityError(String::from(
"Name and size don't match for fixed.",
)))
```
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395856883
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -96,10 +109,14 @@ impl Checker {
symbols: r_symbols, ..
}) = readers_schema
{
- return !w_symbols.iter().any(|e| !r_symbols.contains(e));
+ if !w_symbols.iter().any(|e| !r_symbols.contains(e)) {
Review Comment:
Double negation give me headache ;-)
If i understand well, we must ensure that all element of w_symbols must be known from r_symbols ?
Is this
`if w_symbols.iter().all(|e| r_symbols.contains(e)) {`
would be same or 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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1396906754
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Yeah, that would also have to change. For the compatibility, you can just use a `Result<(), CompatibilityError>`. Looking at the code, it was just a `bool` before. I would avoid from the get go tying it to `AvroResult`.
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1400473454
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,42 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Oh yes!! It is not used at all!
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1401821479
##########
lang/rust/avro/src/error.rs:
##########
@@ -480,6 +480,39 @@ pub enum Error {
BadCodecMetadata,
}
+#[derive(thiserror::Error, Debug)]
+pub enum CompatibilityError {
+ #[error("Schemas are not compatible. Writer schema is {writer_schema_type}, but reader schema is {reader_schema_type}")]
+ WrongType { writer_schema_type: String, reader_schema_type: String},
+
+ #[error("Schemas are not compatible. The {schema_type} should have been {expected_type}")]
+ TypeExpected {schema_type: String, expected_type: String},
+
+ #[error("Schemas are not compatible. Field '{0}' in reader schema does not match the type in the writer schema")]
+ FieldTypeMissmatch(String),
+
+ #[error("Schemas are not compatible. Schemas missmatch")]
+ SchemaMissmatch,
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1396906754
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Yeah, that would also have to change. For the compatibility, you can just use a `Result<(), CompatibilityError`. Looking at the code, it was just a `bool` before. I would avoid from the get go tying it to `AvroResult`.
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395612123
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -133,38 +156,64 @@ impl Checker {
{
for field in r_fields.iter() {
if let Some(pos) = w_lookup.get(&field.name) {
- if !self.full_match_schemas(&w_fields[*pos].schema, &field.schema) {
- return false;
+ if self
+ .full_match_schemas(&w_fields[*pos].schema, &field.schema)
+ .is_err()
+ {
+ return Err(Error::CompatibilityError(format!("Field {} in reader schema does not match the type in the writer schema", field.name)));
}
} else if field.default.is_none() {
- return false;
+ return Err(Error::CompatibilityError(format!(
+ "Field {} in reader schema must have a default value",
+ field.name
+ )));
}
}
}
}
- true
+ Ok(())
}
- fn match_union_schemas(&mut self, writers_schema: &Schema, readers_schema: &Schema) -> bool {
+ fn match_union_schemas(
+ &mut self,
+ writers_schema: &Schema,
+ readers_schema: &Schema,
+ ) -> AvroResult<()> {
let w_type = SchemaKind::from(writers_schema);
let r_type = SchemaKind::from(readers_schema);
assert_eq!(r_type, SchemaKind::Union);
if w_type == SchemaKind::Union {
Review Comment:
maybe you could do this instead, to reduce the nesting
```rs
if w_type != SchemaKind::Union {
return Err(Error::CompatibilityError(String::from(
"readers_schema should have been Schema::Union",
)));
}
```
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on PR #2587:
URL: https://github.com/apache/avro/pull/2587#issuecomment-1828063839
I have `rebased` from master (it contains the [Fix clippy](https://github.com/apache/avro/pull/2588)) so I think now the CI should work. In my [fork](https://github.com/marcosschroh/avro/pull/1) it seems to work properly
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395510053
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Should this be a separate error? I don't think it's wise to keep adding errors to this `Error`.
As a user, if you try to deserialize and fails, you shouldn't have to handle this error.
```rs
match Schema::parse_str(reader_raw_schema) {
CompatibilityError => unreachable!()
}
```
I think compatibility should have its own enum with a set of errors so the user can react to them properly.
```rs
#[derive(Error, Debug)]
pub enum CompatibilityError {
#[error("We got {input}, but {output} was expected")]
WrongKind { input: SchemaKind, output: SchemaKind},
#[error("Reader's symbols must contain all writer's symbols")]
MissingSymbols,
#[error("Could not calculate compatibility")]
Inconclusive
}
```
And more...
Thoughts @martin-g @marcosschroh ?
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395510053
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Should this be a separate error? I don't think it's wise to keep adding errors to this `Error`.
As a user, if you try to deserialize and fails, you shouldn't have to handle this error.
```rs
match Schema::parse_str(reader_raw_schema) {
CompatibilityError => unreachable!()
}
```
I think compatibility should have its own enum with a set of errors so the user can react to them properly.
```rs
#[derive(Error, Debug)]
pub enum CompatibilityError {
#[error("We got {input}, but {output} was expected")]
WrongKind { input: SchemaKind, output: SchemaKind},
#[error("Reader's symbols must contain all writer's symbols")]
MissingSymbols,
#[error("Could not calculate compatibility")]
Inconclusive
}
```
And more...
Thoughts @martin-g @marcosschroh ?
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395600446
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
Interesting. I am not a rust expert but it seems a good idea. Is the idea that end users could `match` for specific `error` and then have logic for it ? Something like:
```rust
Err(e) => match e.kind() {
WrongKind => break,
MissingSymbols => do_something(),
_ => panic!("Can't read schema"),
},
```
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1395655378
##########
lang/rust/avro/src/schema_compatibility.rs:
##########
@@ -246,80 +308,144 @@ impl SchemaCompatibility {
attributes: _,
}) = readers_schema
{
- return w_name.name == r_name.name && w_size == r_size;
+ if w_name.name == r_name.name && w_size == r_size {
+ return Ok(());
+ } else {
+ return Err(Error::CompatibilityError(String::from(
+ "Name and size don't match for fixed.",
+ )));
+ }
} else {
- unreachable!("readers_schema should have been Schema::Fixed")
+ return Err(Error::CompatibilityError(String::from(
+ "readers_schema should have been Schema::Fixed",
+ )));
}
} else {
- unreachable!("writers_schema should have been Schema::Fixed")
+ return Err(Error::CompatibilityError(String::from(
+ "writers_schema should have been Schema::Fixed",
+ )));
}
}
SchemaKind::Enum => {
if let Schema::Enum(EnumSchema { name: w_name, .. }) = writers_schema {
if let Schema::Enum(EnumSchema { name: r_name, .. }) = readers_schema {
- return w_name.name == r_name.name;
+ if w_name.name == r_name.name {
+ return Ok(());
+ } else {
+ return Err(Error::CompatibilityError(String::from(
+ "Names don't match",
+ )));
+ }
} else {
- unreachable!("readers_schema should have been Schema::Enum")
+ return Err(Error::CompatibilityError(String::from(
+ "readers_schema should have been Schema::Enum",
+ )));
}
} else {
- unreachable!("writers_schema should have been Schema::Enum")
+ return Err(Error::CompatibilityError(String::from(
+ "writers_schema should have been Schema::Enum",
+ )));
}
}
SchemaKind::Map => {
if let Schema::Map(w_m) = writers_schema {
if let Schema::Map(r_m) = readers_schema {
return SchemaCompatibility::match_schemas(w_m, r_m);
} else {
- unreachable!("readers_schema should have been Schema::Map")
+ return Err(Error::CompatibilityError(String::from(
+ "readers_schema should have been Schema::Map",
+ )));
}
} else {
- unreachable!("writers_schema should have been Schema::Map")
+ return Err(Error::CompatibilityError(String::from(
+ "writers_schema should have been Schema::Map",
+ )));
}
}
SchemaKind::Array => {
if let Schema::Array(w_a) = writers_schema {
if let Schema::Array(r_a) = readers_schema {
return SchemaCompatibility::match_schemas(w_a, r_a);
} else {
- unreachable!("readers_schema should have been Schema::Array")
+ return Err(Error::CompatibilityError(String::from(
+ "readers_schema should have been Schema::Array",
+ )));
}
} else {
- unreachable!("writers_schema should have been Schema::Array")
+ return Err(Error::CompatibilityError(String::from(
+ "writers_schema should have been Schema::Array",
+ )));
}
}
- _ => (),
+ _ => {
+ return Err(Error::CompatibilityError(String::from(
+ "Unknown reader type",
+ )))
+ }
};
}
- if w_type == SchemaKind::Int
- && [SchemaKind::Long, SchemaKind::Float, SchemaKind::Double]
+ // Here are the checks for primitive types
+ if w_type == SchemaKind::Int {
Review Comment:
this could be a `match w_type`
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "woile (via GitHub)" <gi...@apache.org>.
woile commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1400362230
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,42 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
This should be either removed (preferred I think), or propagte from the other enum:
```rs
CompatibilityError {
#[from]
source: CompatibilityError
},
```
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on PR #2587:
URL: https://github.com/apache/avro/pull/2587#issuecomment-1825554049
@clesaec Could you approve the workflow? I have update the formatting and the README.md. For the. rest I would like to see whether there are more errors :-)
--
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
Re: [PR] AVRO-3904: [RUST] return a Result when checking schema compatibility so the … [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2587:
URL: https://github.com/apache/avro/pull/2587#discussion_r1398241834
##########
lang/rust/avro/src/error.rs:
##########
@@ -478,6 +478,9 @@ pub enum Error {
#[error("Invalid Avro data! Cannot read codec type from value that is not Value::Bytes.")]
BadCodecMetadata,
+
+ #[error("Schemas are not compatible. '{0}'")]
+ CompatibilityError(String),
Review Comment:
In a separate commit I have added the new error `CompatibilityError` with its types so the users can specify business logic per error type. Some errors type are missing because the current code is not covering uses cases like `logicalTypes` and`enum orders`, we will have to add them in the future as we continue using this library.
--
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