Upgrade DF to 51 #5189
Open
Upgrade DF to 51 #5189
+3,376
−2,336
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AdamGS
added
feature
37 tasks
CodSpeed Performance ReportMerging #5189 will degrade performance by 47.48%Comparing Summary
Benchmarks breakdown
Footnotes
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (NVMe)Summary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (S3)Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
AdamGS
force-pushed
the
adamg/df-51
branch
2 times, most recently
from
e587525 to
07aee41
Compare
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Author
|
So @a10y is on vacation for the next few weeks, so just updating my current thinking here:
|
Contributor
|
I'm also on PTO this week so won't be very reachable but I think most of the urgency comes from some changes @brancz is working on so I'll defer to him. Re the vendored code I don't think I mind it too much, especially since it'll be deleted soon. I think the question is whether it's worth the effort to push through this 51 release if 52 is coming soon and most everyone is on holiday starting next week. |
Contributor
|
My personal bias is to merge the vendored code now since it’s an indication of an upstream bug for a feature we care about. Then delete it once 52 is around. Without it gharchive is broken and it’s a bit annoying to wait for upstream. |
Contributor
|
No strong opinion one way or another, we're already at the point where we're carrying several unreleased patches on upstream for both arrow and datafusion. |
Contributor
|
Jynxed it, I'm quite certain I'm seeing a very noticeable case for us where we're lacking projection pushdown for a field in a struct (which is one of the things this fixes). Would appreciate it if we could merge this. |
Contributor
Author
|
This is currently blocked by #5676, once I merge that (hopefully today), I'll rebase this PR and try and catch up on all the pieces here and get it merged. |
Signed-off-by: Adam Gutglick <[email protected]>
Signed-off-by: Adam Gutglick <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
Signed-off-by: Andrew Duffy <[email protected]>
AdamGS
force-pushed
the
adamg/df-51
branch
3 times, most recently
from
794bcee to
9b8aeb8
Compare
Signed-off-by: Adam Gutglick <[email protected]>
Contributor
Author
|
I've rebased this PR and fixed a couple of small random issues that came up, the codspeed regressions all point back to Arrow, but that might be an artifact of the runtime because the only relevant changes in Arrow have some pretty convincing benchmarks? I'm not worried about them. |
Contributor
Author
|
I can't reproduce this TPC-DS issue, if it keeps popping up I'll dig deeper into it. |
Contributor
Author
|
@asubiotto if you want to take look here, @a10y is OOO until the 7th IMO, not sure if we have anyone else that can weigh in. |
Contributor
Author
|
finally managed to get a repro of the TPC-DS issue, it has something to do with the new decimal types, I think we should fix that before we merge this, so I'll keep working on it. |
Contributor
Author
|
Fixing TPC-DS here might require a deeper fix in DF, for now I'll remove some of the DF/Decimal code paths |
Signed-off-by: Adam Gutglick <[email protected]>
AdamGS
commented
Jan 2, 2026
| let scale = dt.scale(); | ||
|
|
||
| match precision { | ||
| // This code is commented out until DataFusion improves its support for smaller decimals. |
Contributor
Author
There was a problem hiding this comment.
This will stay for now, I've hit an overflow in TPC-DS q1, and I might take a bigger stab at how to handle it in DF.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing the upgrade to DF 51.
51.0.0(Nov 2025) apache/datafusion#17558 (comment)