apd: don't strip trailing zeros in Quo and Sqrt #115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuzefovich
approved these changes
Jan 31, 2022
There was a problem hiding this comment.
Nice :)
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on @yuzefovich)
Before this commit, `Quo` and `Sqrt` were stripping trailing zeros from
their result. This was different from the behavior in PostgreSQL, where
these operations retain all trailing zeros based on the precision of
the computation:
In PostgreSQL:
```sql
nathan=# select d, v from dec;
d | v
---------+-----
10 | 4
10.0000 | 4.0
(2 rows)
nathan=# select d/v, scale(d/v), trim_scale(d/v) from dec;
?column? | scale | trim_scale
--------------------+-------+------------
2.5000000000000000 | 16 | 2.5
2.5000000000000000 | 16 | 2.5
(2 rows)
nathan=# select sqrt(v), scale(sqrt(v)), trim_scale(sqrt(v)) from dec;
sqrt | scale | trim_scale
-------------------+-------+------------
2.000000000000000 | 15 | 2
2.000000000000000 | 15 | 2
(2 rows)
```
Making this change will improve PG compatibility, especially when we
decide to implement the `scale` and `trim_scale` builtin functions.
Conveniently, removing this also improves performance, as these operations
now perform less work.
```
name old time/op new time/op delta
GDA/exp-10 20.6ms ± 0% 16.2ms ± 1% -21.21% (p=0.000 n=10+10)
GDA/log10-10 25.6ms ± 0% 22.7ms ± 0% -11.57% (p=0.000 n=10+9)
GDA/ln-10 19.9ms ± 0% 17.6ms ± 0% -11.35% (p=0.000 n=10+10)
GDA/divide-10 65.3µs ± 0% 59.4µs ± 0% -9.03% (p=0.000 n=9+9)
GDA/squareroot-10 5.31ms ± 0% 5.01ms ± 0% -5.57% (p=0.000 n=10+8)
GDA/powersqrt-10 73.9ms ± 0% 71.3ms ± 0% -3.48% (p=0.000 n=9+9)
GDA/cuberoot-apd-10 397µs ± 0% 385µs ± 0% -3.05% (p=0.000 n=10+10)
GDA/divideint-10 13.4µs ± 0% 13.4µs ± 0% ~ (p=0.859 n=9+10)
name old alloc/op new alloc/op delta
GDA/divide-10 10.6kB ± 0% 10.6kB ± 0% ~ (all equal)
GDA/divideint-10 96.0B ± 0% 96.0B ± 0% ~ (all equal)
GDA/cuberoot-apd-10 122kB ± 0% 124kB ± 0% +1.81% (p=0.000 n=10+10)
GDA/powersqrt-10 6.27MB ± 0% 6.66MB ± 0% +6.22% (p=0.000 n=10+10)
GDA/log10-10 10.4MB ± 0% 11.4MB ± 0% +10.03% (p=0.000 n=10+10)
GDA/ln-10 7.93MB ± 0% 8.74MB ± 0% +10.22% (p=0.000 n=10+10)
GDA/squareroot-10 147kB ± 0% 170kB ± 0% +15.82% (p=0.000 n=10+10)
GDA/exp-10 9.56MB ± 0% 11.26MB ± 0% +17.71% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
GDA/divide-10 213 ± 0% 213 ± 0% ~ (all equal)
GDA/divideint-10 2.00 ± 0% 2.00 ± 0% ~ (all equal)
GDA/cuberoot-apd-10 2.43k ± 0% 2.48k ± 0% +1.81% (p=0.000 n=10+10)
GDA/powersqrt-10 210k ± 0% 217k ± 0% +3.53% (p=0.000 n=10+10)
GDA/squareroot-10 3.93k ± 0% 4.18k ± 0% +6.36% (p=0.000 n=10+10)
GDA/log10-10 205k ± 0% 220k ± 0% +7.46% (p=0.000 n=8+10)
GDA/ln-10 157k ± 0% 169k ± 0% +7.67% (p=0.000 n=10+10)
GDA/exp-10 127k ± 0% 144k ± 0% +13.27% (p=0.000 n=10+10)
```
nvanbenschoten
force-pushed
the
nvanbenschoten/stripZeros
branch
from
c5599e8 to
04d1483
Compare
Start output on separate line. Improves readability.
|
TFTR! |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Feb 1, 2022
Picks up two PRs that improved the performance of `Quo`, `Sqrt`, `Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`: - cockroachdb/apd#114 - cockroachdb/apd#115 Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`.
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
Feb 2, 2022
Picks up two PRs that improved the performance of `Quo`, `Sqrt`, `Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`: - cockroachdb/apd#114 - cockroachdb/apd#115 Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`.
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
Feb 2, 2022
74612: rangefeedcache,settingswatcher,systemcfgwatcher: lose gossip deps r=ajwerner a=ajwerner This is a pile of commits which supersedes #69269 and pretty much puts in place all of the pieces to move on #70560. 75726: sql: refactor pg_has_role to remove privilege.GRANT usage r=RichardJCai a=ecwall refs #73129 Also combines some layers of privilege checking code. Release note: None 75770: vendor: bump cockroachdb/apd to v3.1.0, speed up decimal division r=nvanbenschoten a=nvanbenschoten Picks up two PRs that improved the performance of `Quo`, `Sqrt`, `Cbrt`, `Exp`, `Ln`, `Log`, and `Pow`: - cockroachdb/apd#114 - cockroachdb/apd#115 Almost all of the testing changes here are due to the rounding behavior in cockroachdb/apd#115. This brings us closer to PG's behavior, but also creates a lot of noise in this diff. To verify that this noise wasn't hiding any correctness regressions caused by the rewrite of `Context.Quo` in the first PR, I created #75757, which only includes the first PR. #75757 passes CI with minimal testing changes. The testing changes that PR did require all have to do with trailing zeros, and most of them are replaced in this PR. Release note (performance improvement): The performance of many DECIMAL arithmetic operators has been improved by as much as 60%. These operators include division (`/`), `sqrt`, `cbrt`, `exp`, `ln`, `log`, and `pow`. ---- ### Speedup on TPC-DS dataset The TPC-DS dataset is full of decimal columns, so it's a good playground to test this change. Unfortunately, the variance in the runtime performance of the TPC-DS queries themselves is high (many queries varied by 30-40% per attempt), so it was hard to get signal out of them. Instead, I imported the TPC-DS dataset with a scale factor of 10 and ran some custom aggregation queries against the largest table (web_sales, row count = 7,197,566): ```sql # q1 select sum(ws_wholesale_cost / ws_ext_list_price) from web_sales; # q2 select sum(ws_wholesale_cost / ws_ext_list_price - sqrt(ws_net_paid_inc_tax)) from web_sales; ``` Here's the difference in runtime of these two queries before and after this change on an `n2-standard-8` instance: ``` name old s/op new s/op delta TPC-DS/custom/q1 22.4 ± 0% 8.6 ± 0% -61.33% (p=0.002 n=6+6) TPC-DS/custom/q2 91.4 ± 0% 32.1 ± 0% -64.85% (p=0.008 n=5+5) ``` 75771: colexec: close the ordered sync used by the external sorter r=yuzefovich a=yuzefovich **colexec: close the ordered sync used by the external sorter** Previously, we forgot to close the ordered synchronizer that is used by the external sorter to merge already sorted partitions. This could result in some tracing spans never being finished and is now fixed. Release note: None **colexec: return an error rather than logging it on Close in some cases** This error eventually will be logged anyway, but we should try to propagate it up the stack as much as possible. Release note: None 75807: ui: Add CircleFilled component r=ericharmeling a=ericharmeling Previously, there was no component for a filled circle icon in the `ui` package. Recent product designs have requested this icon for the DB Console (see #67510, #73463). This PR adds a `CircleFilled` component to the `ui` package. Release note: None 75813: sql: fix flakey TestShowRangesMultipleStores r=ajwerner a=ajwerner Fixes #75699 Release note: None 75836: dev: add generate protobuf r=ajwerner a=ajwerner Convenient, fast. Release note: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Evan Wall <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Eric Harmeling <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this commit,
QuoandSqrtwere stripping trailing zeros fromtheir result. This was different from the behavior in PostgreSQL, where
these operations retain all trailing zeros based on the precision of
the computation:
In PostgreSQL:
Making this change will improve PG compatibility, especially when we
decide to implement the
scaleandtrim_scalebuiltin functions.Conveniently, removing this also improves performance, as these operations
now perform less work.