-
Notifications
You must be signed in to change notification settings - Fork 305
fix: await get catalog in datafusion integration test and update version in python binding #1635
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
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 @lliangyu-lin for this fix!
@@ -27,7 +27,7 @@ | |||
import datafusion | |||
|
|||
assert ( | |||
datafusion.__version__ >= "45" | |||
datafusion.__version__ >= "47" | |||
) # iceberg table provider only works for datafusion >= 45 |
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.
Fix the comments?
Hi @liurenjie1024, I think the python binding error is still not fixed yet. The build is passing though after adding the We can probably merge this to fix the build failure first |
Thanks @lliangyu-lin for the effort. I don't think we should merge this without actually fixing the ci failure. I'll do some investigation to about it. |
@liurenjie1024 I tried revert the insert into support commit and rerun the tests locally, but the tests are still failing. I'm suspecting something to do with the datafusion and arrow versions. Pinning the version to arrow 16 and datafusion 47 seems to fix the issue for now. |
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.
While I don't think it should be that picky on the Arrow version, I think it would be good to get this in to get the CI unblocked. Thanks @lliangyu-lin for working on this, and @liurenjie1024 fro the review 🙌
I think it's due to apache/datafusion-python#1217. We can loosen arrow version after @kevinjqliu getting #1639 in |
Which issue does this PR close?
What changes are included in this PR?
insert_into
forIcebergTableProvider
#1600 added few integration tests, but need to include the new changes introduced in memory catalog loader change feat(catalog): Implement catalog loader for in memory #1623 .Are these changes tested?
Yes, existing tests