-
Notifications
You must be signed in to change notification settings - Fork 30
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
[SQL] Add function exp(double) -> double #1406
Conversation
Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
@Test @Ignore("FP comparison error for Calcite optimized version") | ||
public void testExpEdgeCase() { | ||
this.qs(""" | ||
-- changed the type from numeric to double | ||
-- therefore the value has also slightly changed | ||
-- cases that used to generate inaccurate results in Postgres | ||
select exp(32.999::double); | ||
exp | ||
--------------------- | ||
214429043492155.56 | ||
(1 row) | ||
""" | ||
); | ||
} |
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.
error: test failed, to rerun pass `--lib`
stdout ----
R: ("214429043492155.5625",)x1
L: ("214429043492155.59375",)x1
thread 'test0' panicked at src/lib.rs:151:5:
mvn test -Dtest=FunctionsTest#testExpEdgeCase
-- changed the type from numeric to double
-- therefore the value has also slightly changed
-- cases that used to generate inaccurate results in Postgres
select exp(32.999::double)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
test0
test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Both values are pretty close.
And when compared with float-cmp are approximately equal, between 1 ULP.
let expected = 214429043492155.5625_f64;
let got = 214429043492155.59375_f64;
assert!(expected.approx_eq(got, (f64::EPSILON, 1)));
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.
Can you check if they are from calcite? Then it's the bug I fixed there
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 for the Calcite optimized version. 214429043492155.59375
is what Calcite returns.
// generated optimized code
Tup1::new(F64::new(f64::from_bits(4821209731050809203u64))/*2.144290434921556E14*/)
// the output it is being compared against
Tup1::new(F64::new(f64::from_bits(4821209731050809202u64))/*2.1442904349215556E14*/)
select exp(1234.5678); | ||
exp | ||
----- | ||
146549072930959479983482138503979804217622199675223653966270157446954995433819741094410764947112047906012815540251009949604426069672532417736057033099274204598385314594846509975629046864798765888104789074984927709616261452461385220475510438783429612447831614003668421849727379202555580791042606170523016207262965336641214601082882495255771621327088265411334088968112458492660609809762865582162764292604697957813514621259353683899630997077707406305730694385703091201347848855199354307506425820147289848677003277208302716466011827836279231.9667 |
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.
how is this possible? This exceeds the precision of a Double. Most of these digits cannot be right
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.
Why is this printed with so many digits of precision? doubles should stop at 15
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 copied this from: https://github.com/postgres/postgres/blob/09eb633e1baa3b7cd7929f3cc77f9c46f63c20b1/src/test/regress/expected/numeric.out#L2976
I assume that internally it is ignored after 15 places.
Its surprising that this doesn't give an error like the one above.
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.
you should check the generated rust
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.
The generated code is the same for both optimized and unoptimized versions:
Tup1::new(exp_d(cast_to_d_decimal(new_decimal("1234.5678", 8, 4).unwrap())))
exp_d
returns infinity.
The zset for the output is:
Tup1::new(F64::new(f64::from_bits(9218868437227405312u64))/*std::f64::INFINITY*/) => 1,
Which is also infinity.
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.
Then this is the test we have to run
Signed-off-by: Abhinav Gyawali <22275402+abhizer@users.noreply.github.com>
Is this a user-visible change (yes/no): yes
Partial fix to: #1377
Numeric types are cast to double precision as Calcite has:
Both have the return type double.
There are also some issues with floating point comparison when the precision is too high.