Skip to content

Conversation

lliangyu-lin
Copy link
Contributor

@lliangyu-lin lliangyu-lin commented Aug 29, 2025

Which issue does this PR close?

  • Closes #.

What changes are included in this PR?

Are these changes tested?

Yes, existing tests

@lliangyu-lin lliangyu-lin changed the title fix: await get catalog in integration test fix: await get catalog in datafusion integration test Aug 29, 2025
liurenjie1024
liurenjie1024 previously approved these changes Aug 29, 2025
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the comments?

@lliangyu-lin
Copy link
Contributor Author

Hi @liurenjie1024, I think the python binding error is still not fixed yet. The build is passing though after adding the .await(), but python binding seems like a different error, which might need deeper look.

We can probably merge this to fix the build failure first

@liurenjie1024
Copy link
Contributor

Hi @liurenjie1024, I think the python binding error is still not fixed yet. The build is passing though after adding the .await(), but python binding seems like a different error, which might need deeper look.

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.

@lliangyu-lin
Copy link
Contributor Author

lliangyu-lin commented Aug 31, 2025

@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.

@lliangyu-lin lliangyu-lin changed the title fix: await get catalog in datafusion integration test fix: await get catalog in datafusion integration test and update version in python binding Aug 31, 2025
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.

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 🙌

@Fokko Fokko merged commit 79f28ff into apache:main Sep 1, 2025
17 checks passed
@manuzhang
Copy link
Member

I think it's due to apache/datafusion-python#1217. We can loosen arrow version after @kevinjqliu getting #1639 in

@lliangyu-lin lliangyu-lin deleted the integ-df-test-fix branch September 1, 2025 05:03
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.

4 participants