-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update to apache-avro 0.17, fix compatibility changes schema handling #13727
base: main
Are you sure you want to change the base?
Update to apache-avro 0.17, fix compatibility changes schema handling #13727
Conversation
--- updated-dependencies: - dependency-name: apache-avro dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>
a78e358
to
55ca31b
Compare
- Handle ArraySchema struct - Handle MapSchema struct - Map BigDecimal => LargeBinary - Map TimestampNanos => Timestamp(TimeUnit::Nanosecond, None) - Map LocalTimestampNanos => todo!() - Add Default to FixedSchema test
55ca31b
to
9174983
Compare
…/apache-avro-0.17
@@ -144,14 +148,17 @@ fn schema_to_field_with_props( | |||
AvroSchema::Decimal(DecimalSchema { | |||
precision, scale, .. | |||
}) => DataType::Decimal128(*precision as u8, *scale as i8), | |||
AvroSchema::BigDecimal => DataType::LargeBinary, |
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 wondered if there would be a more useful mapping for BigDecimal
, but it seems like binary is good for now:
https://docs.rs/apache-avro/latest/apache_avro/schema/enum.Schema.html#variant.BigDecimal
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 tend to agree, it's not very clear what would be the best mapping.
If it has the preference, we can also set it to "todo!()" that was done for some other types.
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.
Thank you very much @mdroogh
I merged this PR up from main and reaimed it at main to update avro.
Much appreciated
Thanks! @alamb For next time, should I just point the MR to |
Yes, please! |
Which issue does this PR close?
Closes #13588.
Rationale for this change
Making sure apache-avro can be bumped to version 0.17.
What changes are included in this PR?
Are these changes tested?
Not really, making the code compile.
Are there any user-facing changes?
Not really.