remove `-DLONGDOUBLE_TYPE=double` from `Makefile` #337

brodycj · 2020-02-03T21:10:04Z

in order to resolve the long double issue reported in #325

How tested:

Verified that the test proposed in PR [DRAFT ...] test super-large double value ref: issue #325 #336 succeeds on rebuild of dist/sql-asm.js and dist/sql-wasm.js
Verified that running SELECT 1.7976931348623157e+308 in examples/GUI web page with rebuild of dist/worker.sql-wasm.js works correctly

Thanks to @AnyhowStep for raising issue #325 and pointing out the suspect -DLONGDOUBLE_TYPE=double flag in https://github.com/kripken/sql.js/issues/325#issuecomment-573239220.

I suspect that this flag was needed in an old version of asm.js, does not seem to be needed in new emsdk version. Note that I have tested with emsdk version 1.39.4; looks like they published emsdk version 1.39.7 pretty recently.

Follow-up steps if this proposal is accepted and merged:

update this repository with rebuild of all artifacts in dist (I think it would be ideal to rebuild with latest emsdk version, if possible)
review and hopefully merge the new test proposed in PR [DRAFT ...] test super-large double value ref: issue #325 #336

in order to resolve the long double issue discussed in sql-js#325

AnyhowStep · 2020-02-03T21:22:41Z

Thank you for looking into this! This has been gnawing at me for a while, now.

lovasoa

Thank you very much for your contribution ! Could you please add a test for this ?

A simple

assert.deepEqual(
    db.exec("SELECT 1.7976931348623157e+308 as a"), 
    [{columns:['a'],values:[[1.7976931348623157e+308]]}]
);

would be enough.

Also, does this impact performance ?

brodycj · 2020-02-04T15:39:44Z

Could you please add a test for this ?

I encountered 2 problems:

Exact match of the number value did not work for me, likely due to the characteristics of binary encoding of decimal numbers.
The test case I proposed in PR [DRAFT ...] test super-large double value ref: issue #325 #336 succeeds in my work area but will not succeed in GitHub CI until someone rebuilds the dist artifacts. I thought this info was in the description, evidently not clear enough.

I would recommend maintainers consider merging Makefile changes proposed here and in #339 (SQLite 3.31.1 update), rebuild dist, then we can update and merge PR #336 with the added test case.

Also, does this impact performance ?

I cannot say anything for 100% sure without some kind of benchmarking or other measurement. And performance measurements can be a bit challenging to get right due to the number of possible factors that can lead to variance.

I would be very, very surprised if this change could lead to any significant impact, though. The increased amount of generated code should be extremely small in comparison to the amount of processing code that would be used to store and retrieve this kind of data in SQLite, and it should not introduce any new loops.

lovasoa · 2020-02-04T15:47:26Z

I'm not sure I understand what you are saying about the tests. Can you please add tests to this PR ? The github CI should be re-building dist before launching the tests, so the fact that dist is not updated in the PR itself should not matter.

About the numbers that do not match: either the number that is returned matches exactly what was was input and #325 is fixed, or it does not and the issue is still present. In what case are we ?

…nto fix-long-double-issue

brodycj · 2020-02-04T18:49:10Z

Looks like test is now included. To clarify about the exact match: it will not work due to the super-high precision, and I think this is very well known. So I am testing the following for both inline SELECT and SELECT with parameter:

returned value is greater than 1.7976931348623e+308
returned value is less than 1.797693134862316e+308
returned value is not equal to Infinity

I think and hope this is enough to prove that this behavior is now correct and will be tested moving forward.

Please let me know if there is anything else I need to do to get this proposal ready to merge. Thanks!

lovasoa · 2020-02-04T19:32:21Z

I included the tests but they do not pass (see CI).

There should not be any precision loss. What is the exact value that is returned and why is it not exactly the input value ?

Also, testing that the returned value is less than 1.797693134862316e+308 is not necessary. In javascript 1.797693134862316e+308 === Infinity

brodycj · 2020-02-04T20:01:51Z

I don't have much more extra time to investigate this right now. I think I have already demonstrated that my proposal does improve the situation.

lovasoa · 2020-02-04T21:44:40Z

Ok, thank you for what you have done already @brodybits ! I'll try to spend some time on this to make it work later.

brodycj · 2020-03-12T20:38:33Z

I think the precision loss observed here is not due to this library and maybe not even due to JavaScript itself, see this photo:

from this tweet:

Go home JavaScript...you've done enough pic.twitter.com/c8orjpz7W4
— Mike Hartington ([...]) March 12, 2020

lovasoa · 2020-03-12T20:52:36Z

@brodybits What you are showing is just a very well known property of floating point numbers. It has nothing to do with what we are dealing with here: both javascript and sqlite have the notion of a 64-bits IEEE754 number, so it should be possible to get the values from one to the other without any precision loss.

brodycj · 2020-03-12T21:16:51Z

For 64-bit integer values, I would definitely agree that we should not suffer from loss of precision. But I can still see potential for loss of precision on 64-bit floating point numbers. I found interesting sections in a couple resources from a quick search:

Maybe I am arguing too much and thinking too little, but I am really not surprised by this kind of rounding error. In general, floating points are stored with the fractional part in 2s complement, while human-readable fractional part would be in 10s complement. My understanding from many many years ago is that there is no way to prevent rounding issues between 2s complement and 10s complement, except for using excessive precision beyond what we are really interested in. I wish I had more time to research, investigate, and explain this better.

For now, I will just use this proposal in my own build since I think removing -DLONGDOUBLE_TYPE=double does give improved precision with minuscule percentage resource cost. Sorry that my answers are more argumentative than properly researched and thought out here.

AnyhowStep · 2020-03-13T17:48:43Z

Except, they both use the same format to represent the double data type. And they can both parse and perform math on the numbers with the same answer.

I have an idea I'll need to test when I get home.

sql.js : INSERT INTO myTable VALUES (1.7976931348623157e+308)
sql.js : Export to database file
Any other SQLite implementation : Import database file
SELECT * FROM myTable

If it returns 1.7976931348623157e+308, then we know the problem happens when sending values from the wasm to JS.

If it returns Infinity, then we know the wasm itself is parsing the string 1.7976931348623157e+308 as Infinity before storing it.

For reference, db-fiddle SQLite can handle the number just fine,
https://www.db-fiddle.com/f/kGBw8zdD9oJ8EYXBKHT2zV/0

kaizhu256 · 2020-03-13T19:01:56Z

tried out @AnyhowStep 's experiment both ways in sql.js <-> python. the value ends up as infinity in both.

save db in sql.js -> import in python
save db in python -> import in sql.js

AnyhowStep · 2020-03-13T19:40:23Z

1.7976931348623157e+308
Not
1.797693134862316e+308

1.797693134862316e+308 evaluating to Infinity is fine.
I made a typo when making the original comment on my phone. Sorry =(

AnyhowStep · 2020-03-14T01:05:27Z

sqlite3 binary to sql.js

sqlite> CREATE TABLE T (version VARCHAR, f REAL);
sqlite> INSERT INTO T VALUES (sqlite_version(), 1.7976931348623157e+308);
sqlite> .save sqlite-1.7976931348623157e+308.db
sqlite> .exit

version	f
3.31.1	1.7976931348623157e+308

sql.js to sqlite3 binary

sqlite> .open ../Downloads/sql.js.1.7976931348623157e+308.db
sqlite> SELECT * FROM T;
3.28.0|Inf

So, looks to me like the wasm can load 1.7976931348623157e+308 just fine from a .db file.
But it can't parse that value from a SQL string.

kaizhu256 · 2020-03-14T01:27:18Z

yep, 1.7976931348623157e+308 is finite in javascript, sql.js, python.

fyi, there's a subtle sqlite-gotcha, where if you inline it (or CAST from string), its infinity:

you can reproduce above by going to https://kaizhu256.github.io/demo-sqljs-csv/ and copy-paste following code in dev-console:

var worker = new Worker("https://kaizhu256.github.io/demo-sqljs-csv/assets.sqljs-v2019.11.5.rollup.js");
worker.onmessage = function (evt) {
    console.log(JSON.stringify(evt.data.results[0], undefined, 4));
}
worker.postMessage({
    action: "exec",
    params: {
        "@str": "1.7976931348623157e+308",
        "@num": 1.7976931348623157e+308,
},
    sql: `
DROP TABLE IF EXISTS myTable;
CREATE TABLE myTable (type TEXT, val REAL);
INSERT INTO myTable (type, val) VALUES ('inline', 1.7976931348623157e+308);
INSERT INTO myTable (type, val) VALUES ('param-string', @str);
INSERT INTO myTable (type, val) VALUES ('param-number', @num);
SELECT * FROM myTable
`
});

/* console-output:
{
    "columns": [
        "type",
        "val"
    ],
    "values": [
        [
            "inline",
            null // Infinity
        ],
        [
            "param-string",
            null // Infinity
        ],
        [
            "param-number",
            1.7976931348623157e+308
        ]
    ]
}
 */

AnyhowStep · 2020-03-14T01:33:13Z

yep, 1.7976931348623157e+308 is finite in ... sql.js ...

Except, it's not.

It's weird that using the @num with the number 1.7976931348623157e+308 works...

Did you do something different with your fork?

kaizhu256 · 2020-03-14T01:40:46Z

@AnyhowStep, it is finite in sql.js. the gotcha is it cannot be inlined in sql-statement:

-- gotcha - infinite (because sqlite has to parse inlined string)
INSERT INTO myTable (type, val) VALUES ('inline', 1.7976931348623157e+308);
-- gotcha - infinite (because sqlite has to cast from string)
INSERT INTO myTable (type, val) VALUES ('param-string', @str);
-- finite
INSERT INTO myTable (type, val) VALUES ('param-number', @num);

AnyhowStep · 2020-03-14T01:43:10Z

Invoking your code on https://sql-js.github.io/sql.js/examples/GUI/ doesn't work,

So, I'm wondering if your fork is somehow different.

kaizhu256 · 2020-03-14T01:59:21Z

@AnyhowStep the difference is that official demo is lagging and doesn't support passing params to workers (see merged commit 632effa). you can clone the gh-pages branch, and apply following patch (and use the debug version of sql.js):

--- a/dist/worker.sql-wasm-debug.js
+++ b/dist/worker.sql-wasm-debug.js
     * @param {string} sql a string containing some SQL text to execute
     * @return {Database.QueryExecResult[]} The results of each statement
     */
-    Database.prototype["exec"] = function exec(sql) {
+    Database.prototype["exec"] = function exec(sql, params) {
         var curresult;
         var stmt;
         if (!this.db) {
...
                 if (pStmt !== NULL) {
                     curresult = null;
                     stmt = new Statement(pStmt, this);
+                    if (params != null) {
+                        stmt.bind(params);
+                    }
                     while (stmt["step"]()) {
                         if (curresult === null) {
                             curresult = {
...
             return postMessage({
                 id: data["id"],
-                results: db.exec(data["sql"])
+                results: db.exec(data["sql"], data["params"])
             });
         case "each":
             if (db === null) {

AnyhowStep · 2020-03-14T02:01:56Z

So.. All that's left is to figure out why inlining causes it to be Infinity on sql.js, but inlining works with the sqlite3 binary?

Also, thanks for taking the time to investigate this! I know not many people get bothered by small precision differences but it's been nagging at me =(

kaizhu256 · 2020-03-14T02:06:40Z

yes, and maybe CAST <str> AS REAL as well

lovasoa · 2020-04-04T18:44:21Z

closed in favor of #381

Christopher J. Brody added 2 commits February 3, 2020 15:44

test super-large double value ref: issue sql-js#325

ae5291e

remove -DLONGDOUBLE_TYPE=double from Makefile

99dc30a

in order to resolve the long double issue discussed in sql-js#325

brodycj mentioned this pull request Feb 3, 2020

SQLite 3.31.1 update in Makefile #339

Merged

1 task

lovasoa requested changes Feb 4, 2020

View reviewed changes

lovasoa mentioned this pull request Feb 4, 2020

[DRAFT ...] test super-large double value ref: issue #325 #336

Closed

lovasoa added 2 commits February 4, 2020 17:07

Merge remote-tracking branch 'origin/test-super-large-double-value' i…

1d5c00c

…nto fix-long-double-issue

Merge remote-tracking branch 'kripken/master' into fix-long-double-issue

568782b

lovasoa force-pushed the master branch from b3211a6 to e902ec2 Compare February 29, 2020 11:03

kaizhu256 mentioned this pull request Apr 3, 2020

- remove -DLONGDOUBLE_TYPE=double from Makefile #381

Merged

lovasoa closed this Apr 4, 2020

brodycj deleted the fix-long-double-issue branch April 5, 2020 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove `-DLONGDOUBLE_TYPE=double` from `Makefile` #337

remove `-DLONGDOUBLE_TYPE=double` from `Makefile` #337

brodycj commented Feb 3, 2020

AnyhowStep commented Feb 3, 2020

lovasoa left a comment

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020 •

edited

Loading

brodycj commented Mar 12, 2020

lovasoa commented Mar 12, 2020

brodycj commented Mar 12, 2020

AnyhowStep commented Mar 13, 2020 •

edited

Loading

kaizhu256 commented Mar 13, 2020

AnyhowStep commented Mar 13, 2020 •

edited

Loading

AnyhowStep commented Mar 14, 2020 •

edited

Loading

kaizhu256 commented Mar 14, 2020

AnyhowStep commented Mar 14, 2020 •

edited

Loading

kaizhu256 commented Mar 14, 2020

AnyhowStep commented Mar 14, 2020

kaizhu256 commented Mar 14, 2020 •

edited

Loading

AnyhowStep commented Mar 14, 2020 •

edited

Loading

kaizhu256 commented Mar 14, 2020

lovasoa commented Apr 4, 2020

remove -DLONGDOUBLE_TYPE=double from Makefile #337

remove -DLONGDOUBLE_TYPE=double from Makefile #337

Conversation

brodycj commented Feb 3, 2020

AnyhowStep commented Feb 3, 2020

lovasoa left a comment

Choose a reason for hiding this comment

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020

brodycj commented Feb 4, 2020

lovasoa commented Feb 4, 2020 • edited Loading

brodycj commented Mar 12, 2020

lovasoa commented Mar 12, 2020

brodycj commented Mar 12, 2020

AnyhowStep commented Mar 13, 2020 • edited Loading

kaizhu256 commented Mar 13, 2020

AnyhowStep commented Mar 13, 2020 • edited Loading

AnyhowStep commented Mar 14, 2020 • edited Loading

kaizhu256 commented Mar 14, 2020

AnyhowStep commented Mar 14, 2020 • edited Loading

kaizhu256 commented Mar 14, 2020

AnyhowStep commented Mar 14, 2020

kaizhu256 commented Mar 14, 2020 • edited Loading

AnyhowStep commented Mar 14, 2020 • edited Loading

kaizhu256 commented Mar 14, 2020

lovasoa commented Apr 4, 2020

remove `-DLONGDOUBLE_TYPE=double` from `Makefile` #337

remove `-DLONGDOUBLE_TYPE=double` from `Makefile` #337

lovasoa commented Feb 4, 2020 •

edited

Loading

AnyhowStep commented Mar 13, 2020 •

edited

Loading

AnyhowStep commented Mar 13, 2020 •

edited

Loading

AnyhowStep commented Mar 14, 2020 •

edited

Loading

AnyhowStep commented Mar 14, 2020 •

edited

Loading

kaizhu256 commented Mar 14, 2020 •

edited

Loading

AnyhowStep commented Mar 14, 2020 •

edited

Loading