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

tree: allow decimals with any number of decimal places #40379

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Aug 30, 2019

Allow decimals of any length, like how Postgres does. This also prevents
errors from happening in some cases like division by infinity.

closes #40327
closes #35191

Release note: None

Allow decimals of any length, like how Postgres does. This also prevents
errors from happening in some cases like division by infinity.

relates to cockroachdb#40327

Release note: None
@rafiss rafiss requested review from maddyblue and a team August 30, 2019 18:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jordanlewis
Copy link
Member

I'm (perhaps baselessly?) worried about performance. I don't know of any good, targeted benchmarks we could use to test this out, but surely there's some decimal SQL benchmarks somewhere in the code that you could try a before/after on?

@rafiss
Copy link
Collaborator Author

rafiss commented Aug 30, 2019

I'll look around for a benchmark. I'm not sure I understand your concern though -- SetString is not called in any sort of tight loop is it?

Also, in the linked issue there was another idea of calling Reduce (stripping trailing zeroes after the decimal place) at the end of constant-folding. That also solves the issue and would pretty clearly be fine for performance.

@jordanlewis
Copy link
Member

I'll look around for a benchmark. I'm not sure I understand your concern though -- SetString is not called in any sort of tight loop is it?

I hadn't actually reviewed the code yet - but I thought this was about changing the decimal representation to use unlimited precision behind the scenes, instead of the limited precision that we currently use. Now that I read the diff, I will take my concern back.

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 3, 2019

thanks for the reviews!

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 3, 2019

Build failed

@rafiss
Copy link
Collaborator Author

rafiss commented Sep 3, 2019

bors r+

craig bot pushed a commit that referenced this pull request Sep 3, 2019
39417: exec: Add framework for vectorized casts. r=rohany a=rohany

This PR adds in a framework for performing casts
within the vectorized engine, along with a few
casts implemented already. Additionally, it implements
a randomized testing framework for the cast operators.

Fixes #39372

Release note: None

40348: backupccl: deflake TestBackupRestorePartitioned test r=lucy-zhang a=lucy-zhang

`TestBackupRestorePartitioned` was flaky because `EXPERIMENTAL_RELOCATE` could
fail if there were other replication changes in progress. This PR wraps those
statements with `SucceedsSoon()`, following the example of other tests which
use `EXPERIMENTAL_RELOCATE`.

Closes #39653.

Release note: None

40379: tree: allow decimals with any number of decimal places r=rafiss a=rafiss

Allow decimals of any length, like how Postgres does. This also prevents
errors from happening in some cases like division by infinity.

closes #40327
closes #35191 

Release note: None

Co-authored-by: Rohan Yadav <[email protected]>
Co-authored-by: Lucy Zhang <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 3, 2019

Build succeeded

@craig craig bot merged commit 303555e into cockroachdb:master Sep 3, 2019
@rafiss rafiss deleted the decimal-setstring-context branch September 3, 2019 17:30
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.

sql: internal error: DECIMAL: could not evaluate sql: decimals with more than 2000 digits aren't supported
4 participants