-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AVRO-3197 Fallback to the 'type' when the logical type does not support the type #1340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVRO-3197 Fallback to the 'type' when the logical type does not support the type #1340
Conversation
|
The PR is ready for review! |
| use avro_rs::{schema::Name, Error, Schema}; | ||
| use lazy_static::lazy_static; | ||
|
|
||
| fn init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init is quite general. It might be a bit clearer if this is named init_logging or something similar to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that it might be used for other needs later.
Too bad Rust Testing does not provide something like before()/after(), beforeAll()/afterAll().
Using ctor crate looks OK-ish (https://stackoverflow.com/questions/58006033/how-to-run-setup-code-before-any-tests-run-in-rust) but it is one more dev-dependency and it helps only for the before() need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ye I agree, or even something like pytests fixtures.
I see your point with possibly adding more to the init, however I often favour separating the inits unless they are necessary inits for all of the tests. In this case it is more of a nitpick comment, so I don't mind you disregarding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also wondering about the name when introducing it, so I am almost convinced to rename it! :-)
Thanks for the review!
|
@iemejia Could you please review this PR ? Thank you! |
Read the complex type recursively. It seems Avro Java may produce {"type": {"type": "string", "avro.java.string": "String"}, "logicalType": "timestamp-millis"}}, i.e. logicalType is on the same level as the outer "type"
…chema type This is how Avro Java behaves.
6051dc8 to
1b80abd
Compare
…rt the type (#1340) * AVRO-3197 Fallback to the 'type' when the logical type does not support the type * AVRO-3197 Better formatting * AVRO-3197 Allow only when the "type" is "string" * AVRO-3197 Handle problematic complex type for date/time logical types Read the complex type recursively. It seems Avro Java may produce {"type": {"type": "string", "avro.java.string": "String"}, "logicalType": "timestamp-millis"}}, i.e. logicalType is on the same level as the outer "type" * AVRO-3197 Make Clippy happy * AVRO-3197 Log a warning when a logical type is not supported by the Schema type This is how Avro Java behaves. (cherry picked from commit bce3866)
Jira
Tests
typeandlogicalTypewill just log a warning and use thetype. Just as Avro Java does.Commits
Documentation