-
Notifications
You must be signed in to change notification settings - Fork 413
Add the rest of the _convert_schema_if_needed calls #2300
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
Conversation
kevinjqliu
left a comment
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.
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
ca92837 to
bf7a5ef
Compare
|
good call. both instances fixed |
bf7a5ef to
26b3533
Compare
f1461e0 to
102e58b
Compare
|
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! |
|
looks like the linter errored |
102e58b to
dc5ae1d
Compare
|
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. |
yea makes sense we can follow up and fix the TODO |
kevinjqliu
left a comment
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.
LGTM
|
Thanks for the PR @rambleraptor |
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?