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

Runtime: Adds decimal256 support #3587

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Runtime: Adds decimal256 support #3587

merged 7 commits into from
Nov 30, 2023

Conversation

k-anshul
Copy link
Member

closes #3528

@k-anshul k-anshul changed the title Runtime: Added decimal256 support Runtime: Adds decimal256 support Nov 30, 2023
@k-anshul k-anshul self-assigned this Nov 30, 2023
Copy link
Collaborator

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on code styling and changes, hvn't tested the changes.

Comment on lines 350 to 355
num, _ := r.Float64()
if math.IsInf(num, 0) {
row[f] = r.FloatString(38)
} else {
row[f] = num
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for numbers fitting in a float64. Is there any chance we can improve precision here?

One idea (haven't tried it, just based on looking at the docs) is:

n := new(big.Float)
n.SetRat(r)
row[f] = n.String()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The if part takes car of outputting numbers of arbitrary precision and width. I chose 38 which is the max precision for decimal256.
I am eagerly trying to see if the number can fit in float64 so that those numbers are output as float instead of varchar. I am assuming if users have problem casting columns in submitted sql they would have problem casting in model as well. Also I am suspecting most columns marked as BigNumeric would rarely need such high precisions.

r := big.NewRat(1, 2)
_, ok := r.SetString("5.7896044618658097711785492504343953926634992332820282019728792003956564819967E+38")
if !ok {
	panic("yellow")
}

fmt.Println(r.FloatString(38)) // prints 578960446186580977117854925043439539266.34992332820282019728792003956564819967

n := new(big.Float)
n.SetRat(r)
fmt.Println(n.SetPrec(38).String()) // prints 5.789604462e+38

k-anshul and others added 6 commits November 30, 2023 12:27
* test

* inital commit

* nit changes

* fix test and linter

* small changes

* Added test for org switch

* changes for output as csv

* added test for service cmd

* linter fix

* test sleep

* increased sleep time to test

* tidy

* test

* added tests for user command

* other commands added with helper

* nit

* removed unwanted comments

* few changes

* nit

* updated printer functions

* nit

* review changes

* nit

* PR changes

* nit

* ci fix
@begelundmuller begelundmuller merged commit 66e0204 into main Nov 30, 2023
4 checks passed
@begelundmuller begelundmuller deleted the decimal256_support branch November 30, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bigquery: Add support for autocast for BigNumeric columns
4 participants