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/12/01 09:35:44 UTC
[PR] AVRO-3904: [Rust] Minor improvements to the new schema compatibility changes [avro]
martin-g opened a new pull request, #2600:
URL: https://github.com/apache/avro/pull/2600
AVRO-3904
## What is the purpose of the change
* Cleanup and improvements for the changes introduced with https://github.com/apache/avro/pull/2587
## Verifying this change
* Build and tests should pass
## 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
Re: [PR] AVRO-3904: [Rust] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1412302944
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Well, I think it will display `Ref`. Because `SchemaKind` is created with [strum_discriminant](https://docs.rs/strum/latest/strum/derive.EnumDiscriminants.html) then the `name` of a `Ref` won't be used. I think an error as `The {schema_type} should have been Ref` is confusing.
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1411921562
##########
lang/rust/avro/src/error.rs:
##########
@@ -480,45 +480,44 @@ pub enum Error {
BadCodecMetadata,
}
-#[derive(thiserror::Error, Debug)]
+#[derive(thiserror::Error, PartialEq)]
pub enum CompatibilityError {
- #[error("Schemas are not compatible. Writer schema is {writer_schema_type}, but reader schema is {reader_schema_type}")]
+ #[error("Incompatible schema types! 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}")]
+ #[error("Incompatible schema types! The {schema_type} should have been {expected_type:?}")]
TypeExpected {
schema_type: String,
- expected_type: String,
+ expected_type: &'static [SchemaKind],
},
- #[error("Schemas are not compatible. Field '{0}' in reader schema does not match the type in the writer schema")]
- FieldTypeMismatch(String),
+ #[error("Incompatible schemata! Field '{0}' in reader schema does not match the type in the writer schema")]
+ FieldTypeMismatch(String, #[source] Box<CompatibilityError>),
- #[error("Schemas are not compatible. Schemas mismatch")]
- SchemaMismatch,
-
- #[error("Schemas are not compatible. Field '{0}' in reader schema must have a default value")]
+ #[error("Incompatible schemata! Field '{0}' in reader schema must have a default value")]
MissingDefaultValue(String),
- #[error("Schemas are not compatible. Reader's symbols must contain all writer's symbols")]
+ #[error("Incompatible schemata! Reader's symbols must contain all writer's symbols")]
Review Comment:
Why do we use the word `schemata`? It is a convention in java?
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1412052751
##########
lang/rust/avro/src/error.rs:
##########
@@ -480,45 +480,44 @@ pub enum Error {
BadCodecMetadata,
}
-#[derive(thiserror::Error, Debug)]
+#[derive(thiserror::Error, PartialEq)]
pub enum CompatibilityError {
- #[error("Schemas are not compatible. Writer schema is {writer_schema_type}, but reader schema is {reader_schema_type}")]
+ #[error("Incompatible schema types! 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}")]
+ #[error("Incompatible schema types! The {schema_type} should have been {expected_type:?}")]
TypeExpected {
schema_type: String,
- expected_type: String,
+ expected_type: &'static [SchemaKind],
},
- #[error("Schemas are not compatible. Field '{0}' in reader schema does not match the type in the writer schema")]
- FieldTypeMismatch(String),
+ #[error("Incompatible schemata! Field '{0}' in reader schema does not match the type in the writer schema")]
+ FieldTypeMismatch(String, #[source] Box<CompatibilityError>),
- #[error("Schemas are not compatible. Schemas mismatch")]
- SchemaMismatch,
-
- #[error("Schemas are not compatible. Field '{0}' in reader schema must have a default value")]
+ #[error("Incompatible schemata! Field '{0}' in reader schema must have a default value")]
MissingDefaultValue(String),
- #[error("Schemas are not compatible. Reader's symbols must contain all writer's symbols")]
+ #[error("Incompatible schemata! Reader's symbols must contain all writer's symbols")]
Review Comment:
It is the used word in the Rust SDK. Consistency is a good thing!
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #2600:
URL: https://github.com/apache/avro/pull/2600
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1411923573
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Cool! Better to use the `Dipslay` trait. Hoe a `Schema::Ref` will be displayed?
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1411923573
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Cool! Better to use the `Dipslay` trait. Who a `Schema::Ref` will be displayed?
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1411923573
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Cool! Better to use the `Dipslay` trait. How a `Schema::Ref` will be displayed?
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "marcosschroh (via GitHub)" <gi...@apache.org>.
marcosschroh commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1412302944
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Well, I think it will display `Ref`. Because `SchemaKind` is created with [strum_discriminant](https://docs.rs/strum/latest/strum/derive.EnumDiscriminants.html) then the `Ref name` won't be used. I think an error as `The {schema_type} should have been Ref` is confusing.
--
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] Minor improvements to the new schema compatibility changes [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2600:
URL: https://github.com/apache/avro/pull/2600#discussion_r1412053140
##########
lang/rust/avro/src/schema.rs:
##########
@@ -206,42 +206,6 @@ impl From<&types::Value> for SchemaKind {
}
}
-// Implement `Display` for `SchemaKind`.
Review Comment:
Try 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