Skip to content

Conversation

rambleraptor
Copy link
Contributor

Closes #2270

This adds the rest of the _convert_schema_if_needed calls.

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Looks like theres one more usage of _convert_schema_if_needed here
And another usage of pyarrow_to_schema here

I think we should pass format_version down in both.

I found these by checking the signature changes in #2294
These 3 functions were changed as well, but i double checked them and they are all good.

_pyarrow_to_schema_without_ids
_ConvertToIceberg
_check_pyarrow_schema_compatible

@rambleraptor
Copy link
Contributor Author

good call. both instances fixed

@kevinjqliu kevinjqliu added this to the PyIceberg 0.10.0 milestone Aug 10, 2025
@rambleraptor rambleraptor force-pushed the issue_2270_2 branch 2 times, most recently from f1461e0 to 102e58b Compare August 18, 2025 17:54
@rambleraptor
Copy link
Contributor Author

Hey @kevinjqliu, sorry this took so long! I was OOO (literally on a boat with poor WiFi...), so I'm just getting to this now. Let me know if there's anything else you need!

@kevinjqliu
Copy link
Contributor

looks like the linter errored

pyiceberg/table/update/schema.py:146: error: Incompatible default for argument "format_version" (default has type "Literal['TableProperties.DEFAULT_FORMAT_VERSION']", argument has type "Literal[1, 2, 3]")  [assignment]

@rambleraptor
Copy link
Contributor Author

Ugh, circular imports! I made a fix with a TODO. The proper solution is to move TableProperties to a separate module. Happy to do that, but that feels bigger than this PR.

@kevinjqliu
Copy link
Contributor

The proper solution is to move TableProperties to a separate module

yea makes sense we can follow up and fix the TODO

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinjqliu kevinjqliu merged commit 610a154 into apache:main Aug 19, 2025
10 checks passed
@kevinjqliu
Copy link
Contributor

Thanks for the PR @rambleraptor

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.

Improve timestamp support
2 participants