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

Precision on large decimals is lost on select-query (DB contains correct values) #106

Closed
olsn opened this issue Jul 27, 2019 · 9 comments
Closed

Comments

@olsn
Copy link

olsn commented Jul 27, 2019

When doing a simple select-query on a column with a large decimal (e.g. 1234567891011.1213141), everything above 17 digits is cut (most likely the javascript limit here).
The only solution that I see is to return the content as a string instead of a number. - That way the user can decide for themselves if they wish to use some js-library for big numbers.

Here is some code to reproduce:

const config = require("../utils/config-loader").config;  // just db-credentials
const mssql = require("mssql/msnodesqlv8");

async function run() {
	await mssql.connect(config.mssqlConnectionString);
	const sqlQuery = `
		CREATE TABLE [dbo].[TestLargeDecimal] (
				id VARCHAR(12) NOT NULL,
				testfield DECIMAL(21,7) NOT NULL,
				PRIMARY KEY (id)
		);`;
	await mssql.query(sqlQuery);
	
	await mssql.query(`INSERT INTO [dbo].[TestLargeDecimal] (id, testfield) VALUES (1, 1234567891011.1213141);`);
	const result = await mssql.query(`SELECT * FROM [dbo].[TestLargeDecimal]`);
	console.log(result);
	// --> testfield is missing the last 3 digits and only returns "1234567891011.1213"
	// --> the database contains the correct value
	// --> it doesn't actually matter where the decimal-point is, it's probably simply the javascript max-number-length
	
	// optional cleanup
	// await mssql.query(`DROP TABLE [dbo].[TestLargeDecimal];`);
	await mssql.close();
}

run();

If it is in the C-Code, I can probably not help much here, if it's in the Javascript, you can point me to the right direction and I can possibly find a bugfix.

@TimelordUK
Copy link
Owner

can we not just cast the decimal in server to a string?

 const numString = '1234567891011.1213141'
select id, cast(testfield as varchar(${numString.length})) as big_d_as_s from TestLargeDecimal`
test('test retrieving a large decimal as a string', testDone => {
  const precision = 21
  const scale = 7
  const numString = '1234567891011.1213141'
  const fns = [
    asyncDone => {
      theConnection.queryRaw('DROP TABLE TestLargeDecimal', () => {
        asyncDone()
      })
    },
    asyncDone => {
      theConnection.queryRaw(`CREATE TABLE TestLargeDecimal (
        id VARCHAR(12) NOT NULL,
        testfield DECIMAL(${precision},${scale}) NOT NULL,
        PRIMARY KEY (id)
        )`,
      e => {
        assert.ifError(e)
        asyncDone()
      })
    },
    asyncDone => {
      theConnection.query(`INSERT INTO [dbo].[TestLargeDecimal] (id, testfield) VALUES (1, ${numString})`,
        e => {
          assert.ifError(e)
          asyncDone()
        })
    },

    asyncDone => {
      theConnection.query(`select id, cast(testfield as varchar(${numString.length})) as big_d_as_s from TestLargeDecimal`, (e, r) => {
        assert.ifError(e)
        assert.strictEqual(numString, r[0].big_d_as_s)
        asyncDone()
      })
    }
  ]

  async.series(fns, () => {
    testDone()
  })
})

@olsn
Copy link
Author

olsn commented Jul 28, 2019

True, that looks like a usable workaround. - Thank you for that hint!


However, imho this will be a quite ugly workaround, I don't believe that this should be any logic that should be handled outside the DB-Driver.
This is extremely case-specific and you'd have to manually decide which columns to cast and which do not need casting. Other drivers return BigInt e.g. as string regardless of the length, also I know it from other systems (like OData or HANA) there you will get all decimals as String by default.
The default without any custom logic should always return the full & complete data (regardless of the format), that can then be used by the developer regarding the needs. But it should not be the case that the developer first has to implement some special-solution/workaround just to receive the complete data.

@dhensby
Copy link
Contributor

dhensby commented Apr 19, 2022

There's been a bug report in node-mssql about this now (loss of precision for BigInt types).

Due to precision loss in JS the value from the database should not be returned as a native Integer because BigInts are likely to be too large to store without precision losses. As such they should either be returned as strings or possibly native BigInt values.


edit: PR here showing the failure tediousjs/node-mssql#1387

@TimelordUK
Copy link
Owner

I have not tested very much or yet released but have added a switch on connection or query or pool to return numerics as strings - this is done right down in lowest cpp code before data is cast into JS based objects

its checked in on master

 test('query a numeric - configure query to return as string', testDone => {
    async function runner () {
      const num = '12345678.876000'
      const q = `select ${num} as number`
      const res = await theConnection.promises.query({
        query_str: q,
        numeric_string: true
      })
      try {
        assert.deepStrictEqual(res.first[0].number, num)
      } catch (e) {
        assert.ifError(e)
      }
    }
    runner().then(() => {
      testDone()
    })
  })

  test('query a numeric - configure connection to return as string', testDone => {
    async function runner () {
      const num = '12345678.876000'
      theConnection.setUseNumericString(true)
      const q = `select ${num} as number`
      const res = await theConnection.promises.query(q)
      try {
        assert.deepStrictEqual(res.first[0].number, num)
      } catch (e) {
        assert.ifError(e)
      }
    }
    runner().then(() => {
      testDone()
    })
  })

@dhensby
Copy link
Contributor

dhensby commented Apr 25, 2022

I've installed the master branch locally and am seeing BigInt values returned with decimals, but also the value is off by 1, which is interesting.

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '9223372036854775808.000000'
- '9223372036854775807'

That's the result from the tests in my previously linked PR when testing against master

@TimelordUK
Copy link
Owner

can you try master again i have only built windows presently

this will only work with a specific bigint type as below, without cast the type is assumed numeric and bigint cannot be properly held in that type. So bigint cols should work or as below

  test('query a bigint - configure query to return as string', testDone => {
    async function runner () {
      const num = '9223372036854775807'
      const q = `SELECT CAST(${num} AS bigint) as number`
      const res = await theConnection.promises.query({
        query_str: q,
        numeric_string: true
      })
      try {
        assert.deepStrictEqual(res.first[0].number, num)
      } catch (e) {
        assert.ifError(e)
      }
    }
    runner().then(() => {
      testDone()
    })
  })

@TimelordUK
Copy link
Owner

I have yet again had a go at this - and now simply instruct driver to read as strings to make things simpler - i have uploaded again but I have done minimal testing so far. My tests are in params.js if you wish to see what I have tested

@dhensby
Copy link
Contributor

dhensby commented Apr 28, 2022

I've just run master locally (fb56dbd) and my tests are passing locally :)

@TimelordUK
Copy link
Owner

Ok I’m closing this as fixed

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

No branches or pull requests

3 participants