Skip to content

Conversation

@feniljain
Copy link
Contributor

Issue Resolved

Closes #753

About

  • Converted Manifest spec's key_metadata field to be Option<Vec<u8> instead of just Vec<u8>
  • Updated tests to reflect the same
  • Ran both integration tests and iceberg crate tests, all are passing
  • Also ran test: append partition data file #742 to make sure avro file is written correctly and it does fix the underlying issue

@feniljain feniljain force-pushed the fix_key_metadata branch 2 times, most recently from 7150d4c to d45f451 Compare December 14, 2024 06:49
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @feniljain, thanke you for working on this. Here are some minor suggestions.

* test: use `None` instead of `Some` for key_metadata fields
Copy link
Member

@Xuanwo Xuanwo 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, mostly LGTM!

Copy link
Member

@Xuanwo Xuanwo 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 @feniljain for working on this, let's move!

@Xuanwo Xuanwo merged commit 0ba444f into apache:main Dec 14, 2024
16 checks passed
@feniljain feniljain deleted the fix_key_metadata branch December 14, 2024 10:28
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.

Write null for key_metadata instead of empty bytes

2 participants