-
Notifications
You must be signed in to change notification settings - Fork 247
build: Switch back to official DataFusion repo and arrow-rs after Arrow Java 16 is released #403
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
3909ccf to
29c89bf
Compare
|
The fix (apache/datafusion#10094) is not released yet. So the following error still happens in CI: Need to wait for next release. |
|
Besides, the Java Arrow bug is supported to be fixed in Arrow Java 16. But I still see it happens in a few pipelines: And more interesting is, the test doesn't always have the failure. I also cannot reproduce it locally. BTW, it also only failed on ubuntu pipelines, mac os-14 pipelines are all okay without the C Data interface error. |
225a9f3 to
cb2275b
Compare
|
Hmm, the error is actually different: Previously, the fixed one is the buffer at position 1 (offset buffer): I need to look at what the buffer is at position 2 and why Java Arrow passes a null buffer again... |
cb2275b to
7740240
Compare
|
Ah, I found that I made a mistake in the Java Arrow PR that it doesn't initiate the offset buffer well. Proposed another issue at Java Arrow apache/arrow#41609 I found it by trying to dump the buffer value: |
|
I fixed the issue at arrow-rs in the PR apache/arrow-rs#5756. We can continue this PR after the fix is released. Keep this as draft for now. |
|
I have verified the fix apache/arrow-rs#5756 can work by cherry-picking it in my forked branch of arrow-rs. We only need to wait for new release of arrow-rs and DataFusion. |
Do we have an estimate on when new releases of arrow-rs and DataFusion will be available? I'm asking because I'm wondering whether to include this in the first release of Comet. It seems correct to use the released version of arrow-rs and DataFusion in the official release of Comet. |
|
arrow-rs and DataFusion have fast release cycle. We can hold Comet release after the new releases of arrow-rs and DataFusion. |
Thanks for the clarification. I second this. |
…w-rs" This reverts commit 29c89bf.
1ea24a9 to
61072d1
Compare
|
I changed from my forked of DataFusion repo to official DataFusion repo in |
61072d1 to
41d9ffd
Compare
|
@andygrove I can changed to use official DataFusion repo, but cannot do same for arrow-rs repo. The official DataFusion repo uses arrow-rs release 51.0.0. If I point to arrow-rs repo, rust compiler will complain that two different versions of crate of arrow are used. But we need one merged patch in arrow-rs which is not released yet. Thus we still need to wait for next arrow-rs and DataFusion releases. |
41d9ffd to
4b320e3
Compare
|
Switching back to latest DataFusion release/repo causes two issues right now:
|
8050e9e to
f51326b
Compare
|
Okay. After #511 is fixed, there is only one failures: which is known and we already fixed it at the upstream. We only need to wait for next DataFusion and arrow-rs releases that should be happened soon. |
|
Using 39.0.0-rc1 tag 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.
LGTM pending ci
|
All tests are passed! |
|
cc @andygrove |
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.
🎉
…ow Java 16 is released (apache#403) * build: Switch back to released version of DataFusion and arrow-rs * Exclude all arrow dependencies from Spark * Revert "build: Switch back to released version of DataFusion and arrow-rs" This reverts commit 29c89bf. * Test * Test * Test arrow-rs fix * Fix * Use DataFusion repo * Fix * Fix * Use 39.0.0-rc1
Which issue does this PR close?
Closes #248.
Rationale for this change
What changes are included in this PR?
How are these changes tested?