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

fix: precision lost with bigint in sqlite3 #5482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KlausBai
Copy link
Contributor

add supportBigNumbers, bigNumberStrings options like mysql to support sqlite can read bigint by string

add supportBigNumbers, bigNumberStrings options like mysql to support sqlite can read bigint by string
@coveralls
Copy link

Coverage Status

Coverage: 92.392%. Remained the same when pulling cb1c067 on KlausBai:master into 0d27bcb on knex:master.

@KlausBai
Copy link
Contributor Author

KlausBai commented Feb 10, 2023

In my case, i had a table named 'addresses' with schema like this
image
with data
image
When i try to get this row by below code , I find that the precision of 'test' is just lost.

const knex = require('../knex')

const instance = knex({
    client: 'better-sqlite3',
    connection: {
      filename: './data.db',
    },
    useNullAsDefault: true
  })

const conn = instance('addresses');

conn.select('*')
conn.first();
conn.where('id',1);
conn.then(res=>console.log({res}))

image

so I try to add supportBigNumbers, bigNumberStrings options to support knex return bigint value by 'string' , like knex's mysql module do.

In my version, user can just read bigint data by add "supportBigNumbers", "bigNumberStrings" to connection, to get precise data.

const knex = require('../knex')

const instance = knex({
    client: 'better-sqlite3',
    connection: {
      filename: './data.db',
      supportBigNumbers: true,
      bigNumberStrings: true,
    },
    useNullAsDefault: true
  })

const conn = instance('addresses');

conn.select('*')
conn.first();
conn.where('id',1);
conn.then(res=>console.log({res}))

image

@KlausBai
Copy link
Contributor Author

@kibertoad can you make a review for me ?

@OlivierCavadenti OlivierCavadenti self-assigned this Jul 7, 2023
let response = await statement.all(bindings);
if(this.connectionSettings.supportBigNumbers) {
const bigNumberStrings = this.connectionSettings.bigNumberStrings
const processBigintModeRow = (row) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be simplified with:

const processBigintModeRow = (row) => {
  return mapValues(row, v => {
    if (typeof v === 'bigint') {
      const numV = Number(v);
      if (numV === v) {
        return numV;
      } else if (bigNumberStrings) {
        return v.toString()
      }
    }
    return v
  })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this function can be simplified. However, there may be a problem with 'numV === v' because 1n === 1 is false.

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

Successfully merging this pull request may close these issues.

3 participants