Skip to content
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

change SQL numeric literals from float to decimal #3488

Closed

Conversation

kmitchener
Copy link
Contributor

Which issue does this PR close?

Closes #3394.

Rationale for this change

This introduces Decimal type in place of Float for numeric SQL literals.

What changes are included in this PR?

This PR:

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sql SQL Planner labels Sep 14, 2022
@kmitchener
Copy link
Contributor Author

I want to highlight that I've entered issues (tracked in #3480) and commented out tests for some things that broke when using Decimal128. It does feel a bit like Decimal128 has some maturing to do to match the quality of the other numeric datatypes. But I wanted to submit this PR anyway -- if we're committed to making the change to decimal type in this way, then getting it in master is a good way to get additional eyes (and bigger brains than mine) on it.

@codecov-commenter
Copy link

Codecov Report

Merging #3488 (a700e7a) into master (9d028b3) will decrease coverage by 0.04%.
The diff coverage is 93.18%.

@@            Coverage Diff             @@
##           master    #3488      +/-   ##
==========================================
- Coverage   85.75%   85.70%   -0.05%     
==========================================
  Files         299      299              
  Lines       55282    55227      -55     
==========================================
- Hits        47409    47335      -74     
- Misses       7873     7892      +19     
Impacted Files Coverage Δ
datafusion/core/tests/parquet_pruning.rs 99.43% <ø> (ø)
datafusion/core/tests/sql/decimal.rs 100.00% <ø> (ø)
datafusion/core/tests/sql/expr.rs 99.86% <ø> (-0.01%) ⬇️
datafusion/core/tests/sql/select.rs 99.78% <ø> (ø)
datafusion/core/tests/sql/subqueries.rs 94.95% <ø> (ø)
datafusion/sql/src/planner.rs 81.12% <90.00%> (+0.17%) ⬆️
datafusion/core/tests/sql/aggregates.rs 99.35% <100.00%> (-0.03%) ⬇️
datafusion/core/tests/sql/arrow_typeof.rs 100.00% <100.00%> (ø)
...sical-expr/src/aggregate/approx_percentile_cont.rs 69.64% <0.00%> (-11.91%) ⬇️
datafusion/expr/src/binary_rule.rs 84.03% <0.00%> (-0.57%) ⬇️
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jimexist
Copy link
Member

i think this change to UX is a bit too big. can we actually allow some of these functions to auto-convert from decimal to double or vice versa? i don't think asking existing queries to annotate ::double is helpful for adoption in the long term

@alamb
Copy link
Contributor

alamb commented Sep 16, 2022

I want to highlight that I've entered issues (tracked in #3480) and commented out tests for some things that broke when using Decimal128. It does feel a bit like Decimal128 has some maturing to do to match the quality of the other numeric datatypes. But I wanted to submit this PR anyway

THanks @kmitchener

I wonder if we can perhaps mark this PR as a draft as we work through whatever other maturing Decimal needs so that we don't require ::double. I agree with @jimexist that that seems like a step backwards in functionality

@kmitchener
Copy link
Contributor Author

Sure, I will switch it to draft and add another issue for changing the mathematical functions to accept and return decimal types in addition to float, where appropriate. That should reduce the amount of casting needed.

@kmitchener kmitchener marked this pull request as draft September 16, 2022 19:39
@alamb
Copy link
Contributor

alamb commented Sep 17, 2022

Thanks @kmitchener

@alamb
Copy link
Contributor

alamb commented Nov 28, 2023

Closing as this PR is over a year old. Please feel free to reopen it / rebase it if you plan to keep working on it

@alamb alamb closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

change SQL non-integer numeric literal type from Float64 to Decimal128
4 participants