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 literal numeric type from Float64 to Decimal128 #3395

Closed

Conversation

kmitchener
Copy link
Contributor

Which issue does this PR close?

Closes #3394 .

Rationale for this change

What changes are included in this PR?

Per issue, switches SQL non-integer literals to Decimal128 instead of Float64.

Are there any user-facing changes?

@github-actions github-actions bot added the sql SQL Planner label Sep 8, 2022
@kmitchener
Copy link
Contributor Author

@liukun4515 could you take a look at this conversion?

@kmitchener kmitchener changed the title change float to decimal change SQL literal numeric type from Float64 to Decimal128 Sep 8, 2022
@liukun4515
Copy link
Contributor

@liukun4515 could you take a look at this conversion?

I will take a look later

@kmitchener

@andygrove
Copy link
Member

Thanks for working on this @kmitchener.

We really need to be able to parse numeric literals as either float or decimal. We don't need to do all of this in this PR, but we should make sure we file follow-on issues for anything not included here.

Per the ANSI SQL specification:

<signed numeric literal> ::=
[ <sign> ] <unsigned numeric literal>

<unsigned numeric literal> ::=
<exact numeric literal>
| <approximate numeric literal>

<exact numeric literal> ::=
<unsigned integer> [ <period> [ <unsigned integer> ] ]
| <period> <unsigned integer>

<sign> ::= <plus sign> | <minus sign>

<approximate numeric literal> ::= <mantissa> E <exponent>

<mantissa> ::= <exact numeric literal>

<exponent> ::= <signed integer>

<signed integer> ::= [ <sign> ] <unsigned integer>

<unsigned integer> ::= <digit>..

@kmitchener
Copy link
Contributor Author

That's extremely helpful, thanks. What source are you using for the SQL specification?

@kmitchener
Copy link
Contributor Author

In Postgres, these variants all get converted to decimals. https://dbfiddle.uk/CSN4-3op

image

sqlparser-rs is doing something funky with the "e" notation, so I'll add some follow-on issues for that.

@github-actions github-actions bot added the core Core DataFusion crate label Sep 10, 2022
@alamb
Copy link
Contributor

alamb commented Sep 12, 2022

Is this pull request ready for review @kmitchener ? I ask because it is still marked as draft

@kmitchener
Copy link
Contributor Author

Is this pull request ready for review @kmitchener ? I ask because it is still marked as draft

Hi @alamb, no, it's still draft. I've learned a bit since this PR and am figuring out next steps and also working on other PRs that will make this final PR smaller. I can close this and submit a new PR when the change is finally ready if that's preferable? At this point, I need to make more changes before this is ready for review.

@alamb
Copy link
Contributor

alamb commented Sep 12, 2022

. I've learned a bit since this PR and am figuring out next steps and also working on other PRs that will make this final PR smaller. I can close this and submit a new PR when the change is finally ready if that's preferable? At this point, I need to make more changes before this is ready for review.

Sounds good. I am just trying to keep tabs on PRs in this repo to keep them moving, which is why i asked. I'll close this one to signify it doesn't need any additional attention at this time. Thanks for all your help so far @kmitchener

@alamb alamb closed this Sep 12, 2022
@kmitchener kmitchener deleted the 3394-sql-literal-numeric-to-decimal branch September 14, 2022 14:38
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