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

[Bug Fix] Fix TableMetadataV1 Validators #544

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Mar 24, 2024

When working on #498, I noticed that new_table_metadata cannot correctly initialize partition_specs and sort_oders fields for V1 Metadata. For example:

    schema = ... 
    
   partition_spec = PartitionSpec(
        PartitionField(source_id=22, field_id=1022, transform=IdentityTransform(), name="bar"), spec_id=10
    )

    sort_order = SortOrder(
        SortField(source_id=10, transform=IdentityTransform(), direction=SortDirection.ASC, null_order=NullOrder.NULLS_LAST),
        order_id=10,
    )

    actual = new_table_metadata(
        schema=schema,
        partition_spec=partition_spec,
        sort_order=sort_order,
        location="s3://some_v1_location/",
        properties={'format-version': "1"},
    )
--------   
print(actual.partition_specs)
--- output ---
[PartitionSpec(spec_id=0)]

--------
print(actual.sort_orders)
--- output ---
[SortOrder(order_id=0)]

We may update the validators to consider both the field name and its alias. (ex "sort_orders" and "sort-orders") so that the validators won't override any initialized fields.

cc: @Fokko @amogh-jahagirdar

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Oof, this is quite a big one @HonahX. Thanks for catching this. We might want to consider a release to get this bug fix out to the public. WDYT?

@Fokko Fokko merged commit cc8552d into apache:main Mar 25, 2024
7 checks passed
@HonahX
Copy link
Contributor Author

HonahX commented Mar 25, 2024

We might want to consider a release to get this bug fix out to the public. WDYT?

That's a great idea! Shall we create a 0.6.1 milestone or a 0.6.1 proposal in dev-mail-list? I have just opened another bug-fix PR: #547 and I also saw other bug-fix PR may be worth to be added (e.g. #545 ).

BTW, If we finally decide to do the release, I am interested in becoming the release manager. :)

@Fokko
Copy link
Contributor

Fokko commented Mar 25, 2024

@HonahX Yes, that sounds good. There is no formal process for starting a release, just someone needs to take the time to drive the process. Great that you want to become a release manager :D

I would also be open to just running 0.7.0 from the main branch. There is already a lot of cool stuff that would be great to get out to the public.

@Fokko Fokko added this to the PyIceberg 0.6.1 milestone Mar 26, 2024
HonahX added a commit to HonahX/iceberg-python that referenced this pull request Mar 28, 2024
HonahX added a commit that referenced this pull request Mar 28, 2024
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