fix: fix precision bug in parseFloat affecting select(DOUBLE) in prepared statements

[wesgarland]

This corrects Issue #1525 by falling back to the JS engine's own implementation of parseFloat.

[wesgarland]

@wellwelwel Exactly what kind of tests are you looking for, here?

The current code is demonstrably wrong. Testing the implementation of parseFloat() is covered by the V8 team.

[wellwelwel]

@wellwelwel Exactly what kind of tests are you looking for, here?

The current code is demonstrably wrong. Testing the implementation of parseFloat() is covered by the V8 team.

Hi, @wesgarland. As a fix, to effectively check and ensure that the bug does not recur, it's a useful practice, when possible, ensuring automated tests that reproduce the failure and wait for the failure if the proposed correction is not applied. This also helps to ensure that the correction actually fixes the proposed error.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.83%. Comparing base (53c6526) to head (5444909).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1527      +/-   ##
==========================================
+ Coverage   89.78%   89.83%   +0.05%     
==========================================
  Files          86       86              
  Lines       13607    13579      -28     
  Branches     1607     1600       -7     
==========================================
- Hits        12217    12199      -18     
+ Misses       1390     1380      -10     

Flag	Coverage Δ
compression-0	88.94% <100.00%> (+0.05%)	⬆️
compression-1	89.81% <100.00%> (+0.05%)	⬆️
static-parser-0	87.40% <100.00%> (+0.04%)	⬆️
static-parser-1	88.17% <100.00%> (+0.04%)	⬆️
tls-0	89.24% <100.00%> (+0.05%)	⬆️
tls-1	89.60% <100.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wellwelwel]

I forced the current tests to run through this PR. For me, just removing the variables that are no longer used after the fix is fine. I'm going to take a break to carefully read the discussion that led to this PR in #1525.

[wesgarland]

@wellwelwel Thanks for picking this up and running with it. I don't think there was a CI pipeline when I patched this ~ 4 years ago or I would have picked up on the lint errors. Or maybe I just missed it... anyhow.

To summarize "how we got here", I discovered that selecting float64 values from mysql would give different values in node in some circumstances. The reason is that in those circumstances, they are transmitted over the network as ASCII strings, and sidorares wrote his own implementation of parseFloat in JavaScript. This implementation was marginally faster than that the one built into V8 because it cheated, and made mistakes. My patch removes that code, and uses the parseFloat builtin.

Parsing floating point numbers is a very tricky topic, and writing a test to ensure that a given parser does not make mistakes is a technically challenging task. I do think, however, that assuming that the JS engine implements it correctly is a reasonable assumption.

In retrospect, I'm not sure why I chose to use parseFloat() instead of Number(). I should have documented. It's plausible that Number() would be a few nanoseconds faster.

[wellwelwel]

Parsing floating point numbers is a very tricky topic, and writing a test to ensure that a given parser does not make mistakes is a technically challenging task. I do think, however, that assuming that the JS engine implements it correctly is a reasonable assumption.

I agree. If there were to be a method that competes with V8, I think a dedicated library would be more appropriate, since it is not so simple to test and cover all scenarios.