-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix most benchmark query regressions #4037
Conversation
fn decimal_type(_p: u8, _s: u8) -> DataType { | ||
// TODO use decimal_type(p, s) once Decimal is fully supported | ||
// https://github.com/apache/arrow-datafusion/issues/3523 | ||
DataType::Float64 | ||
} |
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.
When we're ready to try decimal again, we just need to update this one function
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.
We could also add a CLI option to switch between float and decimal
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.
This makes sense to me, the decimal support is still work in progress.
FWIW these is ongoing work to properly support decimal upstream, if anyone is willing to help out
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.
Seems reasonable to me 👍
Closing this in favor of #4044 |
Which issue does this PR close?
Closes #4026
Closes #4025
Closes #4023
Rationale for this change
We cannot use decimal types in the benchmark until they are fully implemented. Overflows are causing queries to return very incorrect results and take longer to run because they return too many rows in some cases.
What changes are included in this PR?
Are there any user-facing changes?
No