Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mdroogh
Copy link

@mdroogh mdroogh commented Dec 10, 2024

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?

  • Handle ArraySchema struct in schema_to_field_with_props
  • Handle MapSchema struct in schema_to_field_with_props
  • Map BigDecimal => LargeBinary in schema_to_field_with_props
  • Map TimestampNanos => Timestamp(TimeUnit::Nanosecond, None) in schema_to_field_with_props
  • Map LocalTimestampNanos => todo!() in schema_to_field_with_props
  • Add Default to FixedSchema test

Are these changes tested?

Not really, making the code compile.

Are there any user-facing changes?

Not really.

---
updated-dependencies:
- dependency-name: apache-avro
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
@mdroogh mdroogh force-pushed the dependabot/cargo/main/apache-avro-0.17 branch 2 times, most recently from a78e358 to 55ca31b Compare December 10, 2024 21:51
- Handle ArraySchema struct
- Handle MapSchema struct
- Map BigDecimal => LargeBinary
- Map TimestampNanos => Timestamp(TimeUnit::Nanosecond, None)
- Map LocalTimestampNanos => todo!()
- Add Default to FixedSchema test
@mdroogh mdroogh force-pushed the dependabot/cargo/main/apache-avro-0.17 branch from 55ca31b to 9174983 Compare December 10, 2024 21:59
@alamb alamb changed the title Fix compatibility changes schema handling apache-avro 0.17 Update to apache-avro 0.17 Fix compatibility changes schema handling Dec 11, 2024
@alamb alamb changed the base branch from dependabot/cargo/main/apache-avro-0.17 to main December 11, 2024 12:15
@@ -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,
Copy link
Contributor

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

Copy link
Author

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.

Copy link
Contributor

@alamb alamb left a 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

@alamb alamb changed the title Update to apache-avro 0.17 Fix compatibility changes schema handling Update to apache-avro 0.17, fix compatibility changes schema handling Dec 11, 2024
@mdroogh
Copy link
Author

mdroogh commented Dec 11, 2024

Thanks! @alamb

For next time, should I just point the MR to main straight away?

@alamb
Copy link
Contributor

alamb commented Dec 11, 2024

Thanks! @alamb

For next time, should I just point the MR to main straight away?

Yes, please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants