Uh oh!
There was an error while loading. Please reload this page.
Bump DataFusion to 50 and arrow to 56 - #4577
Merged
Merged
Conversation
AdamGSforce-pushed
the
adamg/patch-datafusion
branch
from
September 9, 2025 13:10
1f1416a to
018f180Compare45 tasks
CodSpeed Performance ReportMerging #4577 will improve performances by 41.79%Comparing Summary
Benchmarks breakdown
|
AdamGS
commented
Sep 9, 2025
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
0ax1
self-requested a review
September 9, 2025 14:36
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
0ax1
reviewed
Sep 9, 2025
| to_arrow_primitive::<Float64Type>(array) | ||
| } | ||
| (Canonical::Decimal(array), DataType::Decimal32(precision, scale)) => { | ||
| if array.decimal_dtype().precision() != *precision |
Contributor
There was a problem hiding this comment.
Is this check too restrictive? Why not check whether the target type can hold at least the same precision and scale? Maybe I'm misreading something here.
array.decimal_dtype().precision() <= *precision
array.decimal_dtype().scale() <= *scale
Contributor
There was a problem hiding this comment.
As in bail if array.decimal_dtype().precision() > *precision.
ContributorAuthor
There was a problem hiding this comment.
I think I agree, I'll think about it a bit more but probably include it
ContributorAuthor
There was a problem hiding this comment.
After spending some more time with with arrow's and datafusion's decimal-handling code, I think we shouldn't change precision and scale here. Keeping it this way guarantees predictable behavior and zero-copy, unless explicitly cast.
ContributorAuthor
Some work here depends on the outcome of apache/datafusion#17489, if DataFusion's support for smaller decimals doesn't improve, I'll have to rethink some stuff here. |
| } | ||
| pub(crate) fn can_be_pushed_down(expr: &PhysicalExprRef, schema: &Schema) -> bool { | ||
| // We currently do not support pushdown of dynamic expressions in DF |
There was a problem hiding this comment.
I think the dynamic filter will be gradually stable in the next few versions. How about recording an issue for this? There may be potential contributors to participate
ContributorAuthor
There was a problem hiding this comment.
I guess I should link to the issue in the code, I've also expanded there a bit about my current understanding of the issue, which might not be correct but probably worth externalizing.
AdamGSforce-pushed
the
adamg/patch-datafusion
branch
from
September 15, 2025 16:19
e407032 to
0a99805CompareSigned-off-by: Adam Gutglick <adam@spiraldb.com>
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
AdamGSforce-pushed
the
adamg/patch-datafusion
branch
from
September 16, 2025 15:47
0a99805 to
88eaa65CompareAdamGS
marked this pull request as ready for review
September 16, 2025 15:50
robert3005
approved these changes
Sep 16, 2025
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
zhuqi-lucas pushed a commit
to massive-com/vortex
that referenced
this pull request
Sep 23, 2025
Just made sure there's no breaking changes coming our way. Includes support for the two new Arrow decimal types (32 and 64 bits), but without converting from Vortex dtype into them. Codspeed improvements are due to a new inline hint on arrow's `BitIterator::next` (found by @0ax1 🥳). Open issues: - [ ] apache/datafusion#17489 - mostly affects if/how much support we want to provide to decimals, or whether we need to provide some other solution for DataFusion. - [ ] Issue with followups - vortex-data#4668 --------- Signed-off-by: Adam Gutglick <adam@spiraldb.com> (cherry picked from commit cc426c8)
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Just made sure there's no breaking changes coming our way. Includes support for the two new Arrow decimal types (32 and 64 bits), but without converting from Vortex dtype into them.
Codspeed improvements are due to a new inline hint on arrow's
BitIterator::next(found by @0ax1 🥳).Open issues: