Skip to content

feature-request: support 64bit bigint #341

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

Open
kaizhu256 opened this issue Feb 7, 2020 · 14 comments
Open

feature-request: support 64bit bigint #341

kaizhu256 opened this issue Feb 7, 2020 · 14 comments

Comments

@kaizhu256
Copy link
Member

not sure how feasible this feature-request is. sqlite3 docs say INTEGERS can be 1, 2, 3, 4, 6, or 8 bytes.

is it possible to:

  1. input - allow 64-bit bigint as a param input? e.g.
db.exec({
    params: {
        "@bigint": 12345678901234567890n
    },
    sql: "SELECT @bigint"
});
  1. output - if integer > Number.MAX_SAFE_INTEGER, then allow option to auto-coerce it to bigint? e.g.
db.exec({
    preserveIntegerPrecision: true,
    sql: "SELECT 1234 AS int, 1234567890123456789 AS bigint"
});
/*
{
    "results": [
        {
            "columns": [
                "int",
                "bigint"
            ],
            "values": [
                [
                    1234,
                    12345678901234567890123456789n
                ]
            ]
        }
    ]
}
 */
@lovasoa
Copy link
Member

lovasoa commented Feb 18, 2020

I'm not sure it's possible. If someone wants to dive into this, I'll be available to review PRs.

@AnyhowStep
Copy link

AnyhowStep commented Mar 8, 2020

The problem with BigInt is compatibility with browsers that do not support BigInt (Safari, and older browsers).

And the other problem is BigInt polyfills for webworkers.

And then message passing the polyfilled BigInt between the main thread and worker threads. (some nasty gotcha's here)

There are some other issues I can't remember at the moment.


I have a hacked version of sql.js that has BigInt support but I haven't bothered to make any PRs or similar because it would be a backwards compat nightmare, even if hiding it behind a flag.


Statement.prototype.get,

                case SQLITE_INTEGER:
                    results1.push(this.getBigInt(field));
                    break;
                case SQLITE_FLOAT:
                    results1.push(this.getNumber(field));
                    break;

The new Statement.prototype.getBigInt,

    Statement.prototype.getBigInt = function getBigInt(pos) {
      if (pos == null) {
          pos = this.pos;
          this.pos += 1;
      }
      //hacky
      const text = sqlite3_column_text(this.stmt, pos);
      return BigInt(text);
    };

When setting the result of a function after invoking a user-defined function,

      if (typeof result == "bigint") {
        //native bigint supported
        if (
          result >= BigInt(bigIntSignedMinDecimalStr) &&
          result <= BigInt(bigIntSignedMaxDecimalStr)
        ) {
          const leftPart = result >> BigInt(32);
          const rightPart = (leftPart << BigInt(32)) ^ result;
          sqlite3_result_int64(cx, Number(rightPart), Number(leftPart));
        } else {
          sqlite3_result_error(cx, "INTEGER value is out of range in "+name+"(); must be between -9223372036854775808 and 9223372036854775807 inclusive", -1);
        }
        return;
      }
      //Lots of helper functions invoked here...
      if (isPolyfilledBigInt(result)) {
        const decimalStr = result.toString();
        if (
          signedDecimalStrGreaterThanOrEqual(decimalStr, bigIntSignedMinDecimalStr) &&
          signedDecimalStrLessThanOrEqual(decimalStr, bigIntSignedMaxDecimalStr)
        ) {
          const binaryStr = signedDecimalStrToBinaryStr(decimalStr);
          const leftPart = binaryStrSignedRightShift(binaryStr, 32);
          const rightPart = binaryStrXor(
            binaryStrLeftShift(leftPart, 32),
            binaryStr
          );
          sqlite3_result_int64(cx, binaryStrToNumber(rightPart), binaryStrToNumber(leftPart));
        } else {
          sqlite3_result_error(cx, "INTEGER value is out of range in "+name+"(); must be between -9223372036854775808 and 9223372036854775807 inclusive", -1);
        }
        return;
      }

When parsing function arguments,

          if (value_type === SQLITE_INTEGER) {
              arg = BigInt(sqlite3_value_text(value_ptr));
          } else if (value_type === SQLITE_FLOAT) {
              arg = sqlite3_value_double(value_ptr);

Also need cwrap for int64,

    var sqlite3_result_int = cwrap("sqlite3_result_int", "", ["number", "number"]);
    var sqlite3_result_int64 = cwrap("sqlite3_result_int64", "", ["number", "number", "number"]);

@AnyhowStep
Copy link

You can sort-of get around this issue by maybe having a flag or something that always returns integers as text if users provide a custom data mapper. Then users can go ahead and use BigInt in their custom data mapper.

And if no custom data mapper is provided, it defaults to the current lossy behaviour of using JS numbers

@mikedawson
Copy link

Hi,

I think it might make sense to look at this again now that most browsers have BigInt support (including Safari). If someone wants to use SQL.js with data that interfaces with other systems (e.g. server side databases etc), then the 64bit integer support becomes even more important.

What about adding a config object to the database with a flag to enable BigInt e.g.

var myDb = new SQL.Database({
   useBigInt: true
})

That flag can also be passed using the WebWorker. I'm no expert on Javascript packaging, but given that most web browsers these days support BigInt, it seems reasonable to restrict bigint support to where it is natively supported. Native bigint is supported on messages to/from the WebWorker.

If BigInt support is behind a simple config flag, default behavior would not change. Ideally there would be a build that uses emscripten's support for BigInt, but for now maybe it's simpler to use text to pass BigInt to/from SQLite. SQLite will convert text to an Integer when the column type affinity is set to being an Integer.

My colleagues and I would be happy to put together a pull request if this is something that makes sense.

@lovasoa
Copy link
Member

lovasoa commented Aug 5, 2021

A pull request to add support for BigInt in Statement.bindValue and creating a new Statement.getBigInt would already go a long way, and be backward compatible. Could you first implement that, @mikedawson ?

@mikedawson
Copy link

@lovasoa I can make a pull request. Adding support for bindBigInt is simple and backwards compatible (the one calling the function decides what parameters to provide).

The backward compatibility problem will be when deciding what kind of result to return based on the type information we receive from SQLite:

    var stmt = db.prepare("SELECT * FROM test");
    while (stmt.step()) console.log(stmt.get());
     */
    Statement.prototype["get"] = function get(params) {
        if (params != null && this["bind"](params)) {
            this["step"]();
        }
        var results1 = [];
        var ref = sqlite3_data_count(this.stmt);
        for (var field = 0; field < ref; field += 1) {
            switch (sqlite3_column_type(this.stmt, field)) {
                case SQLITE_INTEGER:
                case SQLITE_FLOAT:
                    results1.push(this.getNumber(field));
                    break;
                case SQLITE_TEXT:
                    results1.push(this.getString(field));
                    break;
                case SQLITE_BLOB:
                    results1.push(this.getBlob(field));
                    break;
                default:
                    results1.push(null);
            }
        }
        return results1;
    };

CASE_INTEGER in the above switch needs to put a BigInt into the results. That would not be backwards compatible (e.g. what previously returned a Number would now return a BigInt). Javascript doesn't allow mixing BigInt and Number without explicit conversions - so if someone does myResult + 2 (where myResult is a BigInt) it will throw an exception.

I can't see a way of avoiding the need for some kind of flag to tell SQLite.js what type of value to return. I'm open to any alternative ideas or other ways of passing the flag.

@lovasoa
Copy link
Member

lovasoa commented Aug 5, 2021

I am in favor of still returning js numbers in get. You could add a new getWithBigInt method.

@lovasoa
Copy link
Member

lovasoa commented Aug 5, 2021

Or an optional second parameter to get.

@mikedawson
Copy link

I think a second parameter to get should work fine. My colleagues and I will put the PR together soon.

@kileha3
Copy link
Contributor

kileha3 commented Aug 11, 2021

As per @mikedawson comment, we have added BigInt support and here is the pull request. I'll be around to answer any questions and take in review comments. Thanks

@dbrgn
Copy link

dbrgn commented Jul 20, 2022

The README states:

Binding BigInt is still not supported, only getting BigInt from the database is supported for now.

However, this seems to work as intended:

    const stmt1 = db.prepare("INSERT INTO hello VALUES(:id, 'jklo')");
    stmt1.bind({':id': 9007199254740995n});
    stmt1.step();
    stmt1.free();

    const stmt2 = db.prepare('SELECT * FROM hello');
    while (stmt2.step()) {
        this._log.info('Row:', stmt2.get(undefined, {useBigInt: true}));
    }

The database actually contains an id 9007199254740995, even though that number would be mangled to be 9007199254740996 in JS without bigint support.

What's the current status of bigint support? Which use case is not yet covered?

(Also, wouldn't it make sense to enable bigint support on database- or instance-level, instead of per-statement?)

@dbrgn
Copy link

dbrgn commented Jul 20, 2022

@kileha3 maybe you know more, since you implemented partial BigInt support... Are you using this in production? What are the limitations?

@mikedawson
Copy link

Somehow I missed this afterwards. For anyone looking, the main limitation right now is when you are binding a parameter in a query. Because SQLite.js is compiled without 64bit support as far as I remember, when you bind the bigint to a parameter it is actually bound as a string.

If you are doing an insert query e.g. INSERT INTO TABLE(columns) VALUES(?, ?, ?) - this isn't a problem because SQLite will just convert it automatically (based on the type affinity of the column).

If you do a query like WHERE Table.colName = ? - then there's still a problem, because ? will be bound as a string e.g. 2 + 2 != '4'.

As a workaround: you can use CAST(? AS INTEGER) in your SQL.

Sorry for the late reply but maybe helpful for anyone looking later. We are using the bigint support in production.

@lovasoa I could look at this if helpful, not sure what your plans are now that there is an 'official' WASM release from SQLite itself.

@krpatter-intc
Copy link

Am I reading the PR and docs correctly in that this is for all INTEGER data? This means you can't have a column with float/number and another column with a bigint ?

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

No branches or pull requests

7 participants