-
Notifications
You must be signed in to change notification settings - Fork 552
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
add precision parameter to round function #13965
base: master
Are you sure you want to change the base?
Conversation
e6c7ffc
to
b609549
Compare
What is the reason for this deviation? |
In PostgreSQL: In CrateDB:
PostgreSQL15
CrateDB 5.4.0 (PR)
|
server/src/main/java/io/crate/expression/scalar/arithmetic/RoundFunction.java
Outdated
Show resolved
Hide resolved
} | ||
double val = n.doubleValue(); | ||
int numDecimals = nd.intValue(); | ||
return BigDecimal.valueOf(val).setScale(numDecimals, RoundingMode.HALF_UP).doubleValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if it's not overflowing, (round(<val> * 10 ^ numDecimals)) / <val> * 10 ^ numDecimals)
would be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am on the fence if this might not better return a NUMERIC
.
I would think that the primary use cases for this function is some sort of data presentation / reporting use case, where one would want to limit the amount digits in the fractional part. Using a floating point value, doesn't really guarantee this.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if it's not overflowing, (round( * 10 ^ numDecimals)) / * 10 ^ numDecimals) would be faster.
There is both the problem of overflowing and precision loss as e.g. pointed out here. While one could check overflow and precision to fit within the max safe integer bounds of a double, the checks become cumbersome and harder to maintain. As this would be primarily be a perform optimisation and I'd expect this function to be mostly used for presentation purposes after calculation on a final result set, I think it isn't worth it. Therefore I would suggest to stay with the BigDecimal approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly agree on using NUMERIC
instead, any objections? @mfussenegger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if it's not overflowing, (round( * 10 ^ numDecimals)) / * 10 ^ numDecimals) would be faster.
There is both the problem of overflowing and precision loss as e.g. pointed out here. While one could check overflow and precision to fit within the max safe integer bounds of a double, the checks become cumbersome and harder to maintain. As this would be primarily be a perform optimisation and I'd expect this function to be mostly used for presentation purposes after calculation on a final result set, I think it isn't worth it. Therefore I would suggest to stay with the BigDecimal approach.
Is detecting a possible overflow really that complex?
I do not completely follow you argumentation about the actual use-cases of this function as we never know for sure how users are using it.
But I'm also fine on using your approach now, we can follow up here anytime without a user-facing change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above this method falls apart for inputs like round(260.775, 2)
with an expected output of 260.78
100 * 260.775 == 26077.499999999996
Math.round(26077.499999999996) == 26077
26077 / 100 == 260.77 but should be 260.78
so no. No easy way to detect loss of precision
b609549
to
10da948
Compare
46fa828
to
d3f4479
Compare
server/src/main/java/io/crate/expression/scalar/arithmetic/RoundFunction.java
Outdated
Show resolved
Hide resolved
} | ||
double val = n.doubleValue(); | ||
int numDecimals = nd.intValue(); | ||
return BigDecimal.valueOf(val).setScale(numDecimals, RoundingMode.HALF_UP).doubleValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly agree on using NUMERIC
instead, any objections? @mfussenegger
} | ||
double val = n.doubleValue(); | ||
int numDecimals = nd.intValue(); | ||
return BigDecimal.valueOf(val).setScale(numDecimals, RoundingMode.HALF_UP).doubleValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if it's not overflowing, (round( * 10 ^ numDecimals)) / * 10 ^ numDecimals) would be faster.
There is both the problem of overflowing and precision loss as e.g. pointed out here. While one could check overflow and precision to fit within the max safe integer bounds of a double, the checks become cumbersome and harder to maintain. As this would be primarily be a perform optimisation and I'd expect this function to be mostly used for presentation purposes after calculation on a final result set, I think it isn't worth it. Therefore I would suggest to stay with the BigDecimal approach.
Is detecting a possible overflow really that complex?
I do not completely follow you argumentation about the actual use-cases of this function as we never know for sure how users are using it.
But I'm also fine on using your approach now, we can follow up here anytime without a user-facing change
d3f4479
to
70103db
Compare
This comment was marked as resolved.
This comment was marked as resolved.
681065a
to
7f8a4d9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
When multiple functions are already selected as equal sepcific, the next more specific candidate must only remove the one it was compared with. This issue popped with by #13965 as it adds more variants for the same function.
When multiple functions are already selected as equal sepcific, the next more specific candidate must only remove the one it was compared with. This issue popped with by #13965 as it adds more variants for the same function.
7f8a4d9
to
61d329d
Compare
I rebased and updated it. As mentioned in the comments, I believe there's no way to avoid using BigDecimal for rounding unless we're okay with deviating from PostgreSQL's behavior and risking loss of precision. I also prefer to stick with Double as the return type to align with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it looks good. I kind of agree to not return NUMERIC
(at least now) and we could revisit it once the NUMERIC
lands in cratedb with full support.
Let's wait for @mfussenegger's opinion as well.
With NUMERIC storage support now added (#16471) should this return NUMERIC instead? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I'd tend to return NUMERIC
for the round(v, s)
version and leave the existing overload unchanged.
61d329d
to
c74f878
Compare
} | ||
double val = n.doubleValue(); | ||
int numDecimals = nd.intValue(); | ||
if(numDecimals < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to keep the correct scale for round(123.123,-1)
i.e. 120
instead of 1.2E 2
c74f878
to
20ed4d5
Compare
.argumentTypes(DataTypes.DOUBLE.getTypeSignature(), | ||
DataTypes.INTEGER.getTypeSignature()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the first argument type should also be numeric
, or if we should add a second overload to avoid precision loss if the argument is a numeric
with higher precision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a numeric overload and addressed cases involving long values, which could lose precision when cast to double. Could you please take another look?
Summary of the changes / Why this improves CrateDB
Enhanced the
round()
scalar function to allow a second input parameter for rounding precision.When it is specified, the result's type is
double precision
. Notice thatround(number)
andround(number, 0)
return different result types.Implementation deviates from PostgreSQL. PostgreSQL always returns
NUMERIC
forround()
.Is coherent with
trunc()
scalar function.closes #13964
Checklist
CHANGES.txt
for user facing changessql_features
table for user facing changesCHANGES.txt
(E.g. AdminUI)