Two's complement representation for decimals #10051
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
|
Some preliminary benchmark results for decimal addition: |
|
If this changes long decimal representation, how do we address connectors compatibility? |
martint
force-pushed
the
decimal-twos-complement
branch
from
e262860
to
68d4e5d
Compare
I don't think there's a good way around that. We'll probably have to make this a backward compatibility breaking change. It's unfortunate that we chose the current representation when we first introduced decimals, and that we didn't have the facilities to support arbitrary object types other than Slice back then (which seems to introduce non-trivial performance issues). On the positive side, since we're changing the stack type, any connector that doesn't support the new representation will just fail loudly instead of producing incorrect results. There may be ways to do it in a backward compatible way, though -- I haven't spent too much time thinking about it yet, though. If you have any thoughts on how we might do it, please comment. |
martint
force-pushed
the
decimal-twos-complement
branch
from
68d4e5d
to
f122343
Compare
|
How about exposing method like |
|
Not sure how that would work. The type can only support one “stack” and block type |
|
@kokosing actually i am in favor of backwards incompatible approach. |
I am sorry I haven't read the code yet. I was under impression that "stack" type didn't change but only encoding. |
martint
force-pushed
the
decimal-twos-complement
branch
15 times, most recently
from
4af2e9d
to
62e2b15
Compare
|
Update benchmarks: |
martint
force-pushed
the
decimal-twos-complement
branch
from
62e2b15
to
f013a2c
Compare
martint
changed the title
martint
force-pushed
the
decimal-twos-complement
branch
from
f013a2c
to
5ed15c3
Compare
martint
force-pushed
the
decimal-twos-complement
branch
2 times, most recently
from
c83cc0b
to
a902aeb
Compare
skrzypo987
reviewed
Dec 2, 2021
| public static int compare(long leftHigh, long leftLow, long rightHigh, long rightLow) | ||
| { | ||
| int comparison = Long.compare(leftHigh, rightHigh); | ||
| if (comparison == 0) { |
There was a problem hiding this comment.
You mean like this?
int high = Long.compare(leftHigh, rightHigh);
int low = Long.compareUnsigned(leftLow, rightLow);
return high == 0 ? low : high;It's not clear that that's actually generates branchless code (need to check the assembly), and it has extra cos of computing low unconditionally if the VM is not smart enough to move the computation inside the high == 0 check if it's not able to generate branchless code.
There was a problem hiding this comment.
Rather something like:
int highComparison = Long.compare(leftHigh, rightHigh);
int lowComparison = Long.compareUnsigned(leftLow, rightLow);
comparison = highComparison + ~(highComparison & 1) * lowComparison;
But I guess you can put it on the todo list and benchmark it later
There was a problem hiding this comment.
highComparison & 1
That may not work. Long.compare is not guaranteed to return -1, 0 or 1. According to its javadocs, it will return "a value less than 0 if x < y", "a value greater than 0 if x > y".
It would need to be adjusted to something like:
int high = Long.compare(aHigh, bHigh);
int low = Long.compareUnsigned(aLow, bLow);
int mask = ~(high | -high) >> 31;
int comparison = high + (mask & low);
martint
force-pushed
the
decimal-twos-complement
branch
3 times, most recently
from
81a6a9a
to
692eab1
Compare
skrzypo987
reviewed
Dec 6, 2021
core/trino-main/src/main/java/io/trino/operator/scalar/MathFunctions.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/operator/aggregation/TestDecimalAverageAggregation.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/type/LongDecimalType.java
Outdated
Show resolved
Hide resolved
martint
force-pushed
the
decimal-twos-complement
branch
5 times, most recently
from
d281075
to
a07e645
Compare
martint
force-pushed
the
decimal-twos-complement
branch
from
a07e645
to
dbcc6cb
Compare
Both branches call the same method, so one of them is not needed.
martint
force-pushed
the
decimal-twos-complement
branch
from
dbcc6cb
to
8cd1e8c
Compare
dain
approved these changes
Dec 22, 2021
core/trino-main/src/main/java/io/trino/operator/aggregation/DecimalSumAggregation.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/DecimalSumAggregation.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/MathFunctions.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/scalar/MathFunctions.java
Show resolved
Hide resolved
martint
force-pushed
the
decimal-twos-complement
branch
from
8cd1e8c
to
014aef2
Compare
Also, use a dedicated class to represent long decimals instead of Slice.
Avoid using intermediate BigIntegers when decoding long decimals from Parquet and RCBinary
martint
force-pushed
the
decimal-twos-complement
branch
from
014aef2
to
05ac13c
Compare
Closed
|
@martint Excuse me. I have a question. According to you provided micro benchmark. In some cases, the addition, multiply and division operators have regression. Do the trino fixed these issue? or how to fix them. |
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.
No description provided.