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

Explicitly cast double values down to float in Row.GetFloat(). #706

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

lauxjpn
Copy link
Contributor

@lauxjpn lauxjpn commented Sep 25, 2019

Support casting to float for Pomelo.EntityFramworkCore.MySql for MySQL < 8.0.17.
See PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#827 (comment)

Signed-off-by: Laurents Meyer [email protected]

Supports casting to float in Pomelo.EntityFramworkCore.MySql for MySQL < 8.0.17.

Signed-off-by: Laurents Meyer <[email protected]>
@lauxjpn lauxjpn force-pushed the float_conversion branch 3 times, most recently from 9697e5d to 5881931 Compare September 25, 2019 12:56
@lauxjpn
Copy link
Contributor Author

lauxjpn commented Sep 25, 2019

If you are concerned, that the explicit guard against information loss could lead to an InvalidCastException in cases, where the double value is even only a tiny bit larger/smaller than the float.MinValue/float.MaxValue, it could be dropped entirely.
But it would also mean, that a way higher or lower double value would silently be converted to a float.MinValue/float.MaxValue, which might be unexpected.

@lauxjpn lauxjpn closed this Sep 25, 2019
@lauxjpn lauxjpn reopened this Sep 25, 2019
…ks, to ensure no significant loss of information, when casting from double to float.

Signed-off-by: Laurents Meyer <[email protected]>
// Use explicit range checks to guard against that.
return value switch
{
double doubleValue => (doubleValue >= float.MinValue && doubleValue <= float.MaxValue ? (float) doubleValue : throw new InvalidCastException("The value cannot be safely cast to Single.")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test could be if doubleValue is contained in any of the following ranges: [float.MinValue, -float.Epsilon], [0, 0], [float.Epsilon, float.MaxValue]; what do you think?

Copy link
Contributor Author

@lauxjpn lauxjpn Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the difference would be, that double values between -/+ float.Epsilon and 0 would throw.

The question is, if it is expected that a double between float.Epsilon and 0 is being rounded to float.Epsilon or 0. I think it is, because it's just a result of precision-loss.

(But there could probably be made a case against it as well, which would expect those values to end up as float.Epsilon. I think that is imprecise though, and they need to be rounded. And I would assume, that C#/the processor is doing exactly that in the first place.)

I would not throw in any of these cases, because this would be unexpected and a bit random.

The only potential problem I see with the current test is, that multiple arithmetic operations with a double near the float's min or max threshold, could end up pushing the value over the threshold due to precision error and thus end up throwing an InvalidCastException.
But this is an unlikely corner case and I think guarding against an accidental call of GetFloat on a double column, that then leads to a loss of information, is a much more common and severe case and is worth guarding against.

@bgrainger bgrainger merged commit 93e0d96 into mysql-net:master Sep 25, 2019
@lauxjpn lauxjpn deleted the float_conversion branch September 25, 2019 14:56
@bgrainger
Copy link
Member

Fixed in 0.59.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants