From b9ed52fb615cd5092ce0c7ab3ee52a97333b21b2 Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Wed, 18 Dec 2024 10:23:13 -0600 Subject: [PATCH 1/8] fix(client): parse Presto JSON response with custom reviver and BigInts fix #24 --- .nvmrc | 2 +- presto-client/src/client.ts | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/.nvmrc b/.nvmrc index 4a1f488..fb5b513 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -18.17.1 +21.0.0 diff --git a/presto-client/src/client.ts b/presto-client/src/client.ts index 94530c4..e0042e3 100644 --- a/presto-client/src/client.ts +++ b/presto-client/src/client.ts @@ -9,6 +9,10 @@ import { Table, } from './types' +function digitsToBigInt(_: string, value: unknown, { source }: { source: string }) { + return /^\d+$/.test(source) ? BigInt(source) : value +} + export class PrestoClient { private baseUrl: string private catalog?: string @@ -270,7 +274,7 @@ export class PrestoClient { throw new Error(`Query failed: ${JSON.stringify(await response.text())}`) } - const prestoResponse = (await response.json()) as PrestoResponse + const prestoResponse = (await this.prestoConversionToJSON({ response })) as PrestoResponse if (!prestoResponse) { throw new Error(`Query failed with an empty response from the server.`) } @@ -334,6 +338,13 @@ export class PrestoClient { method, }) } + + private async prestoConversionToJSON({ response }: { response: Response }): Promise { + const text = await response.text() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore JSON.parse with a 3 argument reviver is a stage 3 proposal with some support, allow it here. + return JSON.parse(text, digitsToBigInt) + } } export default PrestoClient From 797f9afec3588511e9d4475fc81b9edba5d60c7b Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Thu, 19 Dec 2024 09:53:56 -0600 Subject: [PATCH 2/8] fix(client): improve big int parsing --- presto-client/src/client.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/presto-client/src/client.ts b/presto-client/src/client.ts index e0042e3..d5f65fe 100644 --- a/presto-client/src/client.ts +++ b/presto-client/src/client.ts @@ -10,7 +10,17 @@ import { } from './types' function digitsToBigInt(_: string, value: unknown, { source }: { source: string }) { - return /^\d+$/.test(source) ? BigInt(source) : value + // Ignore non-numbers + if (typeof value !== 'number') return value + + // If not an integer, use the value + // TODO: Check if Presto can return floats that could also lose precision + if (!Number.isInteger(value)) return value + + // If number is a safe integer, we can use it + if (Number.isSafeInteger(value)) return value + + return BigInt(source) } export class PrestoClient { From 1b2a3b7363aeafeac05465357bab06190db76f9d Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Thu, 19 Dec 2024 10:14:26 -0600 Subject: [PATCH 3/8] fix(client): check compatibility with check function --- presto-client/src/client.ts | 25 ++++++++---------------- presto-client/src/utils.ts | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 17 deletions(-) create mode 100644 presto-client/src/utils.ts diff --git a/presto-client/src/client.ts b/presto-client/src/client.ts index d5f65fe..51b4672 100644 --- a/presto-client/src/client.ts +++ b/presto-client/src/client.ts @@ -8,20 +8,7 @@ import { QueryInfo, Table, } from './types' - -function digitsToBigInt(_: string, value: unknown, { source }: { source: string }) { - // Ignore non-numbers - if (typeof value !== 'number') return value - - // If not an integer, use the value - // TODO: Check if Presto can return floats that could also lose precision - if (!Number.isInteger(value)) return value - - // If number is a safe integer, we can use it - if (Number.isSafeInteger(value)) return value - - return BigInt(source) -} +import { parseWithBigInts, isJsonParseContextAvailable } from './utils' export class PrestoClient { private baseUrl: string @@ -351,9 +338,13 @@ export class PrestoClient { private async prestoConversionToJSON({ response }: { response: Response }): Promise { const text = await response.text() - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore JSON.parse with a 3 argument reviver is a stage 3 proposal with some support, allow it here. - return JSON.parse(text, digitsToBigInt) + if (isJsonParseContextAvailable()) { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore JSON.parse with a 3 argument reviver is a stage 3 proposal with some support, allow it here. + return JSON.parse(text, parseWithBigInts) + } else { + return JSON.parse(text) + } } } diff --git a/presto-client/src/utils.ts b/presto-client/src/utils.ts new file mode 100644 index 0000000..082c474 --- /dev/null +++ b/presto-client/src/utils.ts @@ -0,0 +1,39 @@ +/** + * Parses a JSON including bigger numbers into BigInts + * @param _ Key + * @param value Parsed value + * @param context Context with source text + * @returns Parsed object with BigInts where required + */ +export function parseWithBigInts(_: string, value: unknown, { source }: { source: string }) { + // Ignore non-numbers + if (typeof value !== 'number') return value + + // If not an integer, use the value + // TODO: Check if Presto can return floats that could also lose precision + if (!Number.isInteger(value)) return value + + // If number is a safe integer, we can use it + if (Number.isSafeInteger(value)) return value + + return BigInt(source) +} + +/** + * Checks if JSON.parse reviver callback has a context parameter + * See also: + * - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#browser_compatibility + * - https://github.com/tc39/proposal-json-parse-with-source + * + * This implementation is based on suggestion here: + * - https://github.com/tc39/proposal-json-parse-with-source/issues/40 + */ +export function isJsonParseContextAvailable() { + let contextAvailable + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + JSON.parse('"x"', function (key, value, x) { + contextAvailable = typeof x !== 'undefined' + }) + return contextAvailable +} From c933c9ec58c3a53520bc46894292daeb1a2b2bb6 Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Fri, 20 Dec 2024 09:51:02 -0600 Subject: [PATCH 4/8] refactor(client): move compatibility check to custom reviver Also, update sample nestjs app to include custom JSON.stringify replacer --- apps/nest-server/src/app/app.service.ts | 9 +++++++- presto-client/src/client.ts | 12 ++++------ presto-client/src/utils.ts | 30 +++++++++---------------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/apps/nest-server/src/app/app.service.ts b/apps/nest-server/src/app/app.service.ts index 2bfef18..51ba896 100644 --- a/apps/nest-server/src/app/app.service.ts +++ b/apps/nest-server/src/app/app.service.ts @@ -48,7 +48,14 @@ export class AppService { const results = await client.query( `select returnflag, linestatus, sum(quantity) as sum_qty, sum(extendedprice) as sum_base_price, sum(extendedprice * (1 - discount)) as sum_disc_price, sum(extendedprice * (1 - discount) * (1 + tax)) as sum_charge, avg(quantity) as avg_qty, avg(extendedprice) as avg_price, avg(discount) as avg_disc, count(*) as count_order from lineitem where shipdate <= date '1998-12-01' group by returnflag, linestatus order by returnflag, linestatus`, ) - return { columns: results.columns, rows: results.data } + return { + columns: results.columns, + rows: JSON.stringify(results.data, (key, value) => { + if (typeof value !== 'bigint') return value + + return value.toString() + }), + } } catch (error) { return (error as PrestoError).message } diff --git a/presto-client/src/client.ts b/presto-client/src/client.ts index 51b4672..e2a8b56 100644 --- a/presto-client/src/client.ts +++ b/presto-client/src/client.ts @@ -8,7 +8,7 @@ import { QueryInfo, Table, } from './types' -import { parseWithBigInts, isJsonParseContextAvailable } from './utils' +import { parseWithBigInts } from './utils' export class PrestoClient { private baseUrl: string @@ -338,13 +338,9 @@ export class PrestoClient { private async prestoConversionToJSON({ response }: { response: Response }): Promise { const text = await response.text() - if (isJsonParseContextAvailable()) { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore JSON.parse with a 3 argument reviver is a stage 3 proposal with some support, allow it here. - return JSON.parse(text, parseWithBigInts) - } else { - return JSON.parse(text) - } + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore JSON.parse with a 3 argument reviver is a stage 3 proposal with some support, allow it here. + return JSON.parse(text, parseWithBigInts) } } diff --git a/presto-client/src/utils.ts b/presto-client/src/utils.ts index 082c474..5543b81 100644 --- a/presto-client/src/utils.ts +++ b/presto-client/src/utils.ts @@ -1,11 +1,20 @@ /** * Parses a JSON including bigger numbers into BigInts + * This function checks if JSON.parse reviver callback has a context parameter + * and falls back onto the default parsing if not. + * See also: + * - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#browser_compatibility + * - https://github.com/tc39/proposal-json-parse-with-source * @param _ Key * @param value Parsed value * @param context Context with source text * @returns Parsed object with BigInts where required */ -export function parseWithBigInts(_: string, value: unknown, { source }: { source: string }) { +export function parseWithBigInts(_: string, value: unknown, context: { source: string }) { + if (!context) return value // Context is not available, fallback to default parse + const { source } = context + if (!source) return value // Source is not available, fallback to default parse + // Ignore non-numbers if (typeof value !== 'number') return value @@ -18,22 +27,3 @@ export function parseWithBigInts(_: string, value: unknown, { source }: { source return BigInt(source) } - -/** - * Checks if JSON.parse reviver callback has a context parameter - * See also: - * - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#browser_compatibility - * - https://github.com/tc39/proposal-json-parse-with-source - * - * This implementation is based on suggestion here: - * - https://github.com/tc39/proposal-json-parse-with-source/issues/40 - */ -export function isJsonParseContextAvailable() { - let contextAvailable - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - JSON.parse('"x"', function (key, value, x) { - contextAvailable = typeof x !== 'undefined' - }) - return contextAvailable -} From 9cbe8dee97007f3e1b3d17df78a193d01518a02f Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Fri, 20 Dec 2024 10:32:09 -0600 Subject: [PATCH 5/8] docs(readme): update readme to mention the BigInt parsing and precision errors --- presto-client/README.md | 50 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/presto-client/README.md b/presto-client/README.md index 3fcce4e..54cf89d 100644 --- a/presto-client/README.md +++ b/presto-client/README.md @@ -97,11 +97,18 @@ try { ### Additional notes -Additional notes +Additional notes on the `query` method: -- The `query` method is asynchronous and will return a promise that resolves to a PrestoQuery object. -- The `query` method will automatically retry the query if it fails due to a transient error. -- The `query` method will cancel the query if the client is destroyed. +- It's asynchronous and will return a promise that resolves to a PrestoQuery object. +- It will automatically retry the query if it fails due to a transient error. +- It will cancel the query if the client is destroyed. +- \*It parses big numbers with the BigInt JavaScript primitive. If your Presto response includes a number bigger than `Number.MAX_SAFE_INTEGER`, it will be parsed into a bigint, so you may need to consider that when handling the response, or serializing it. + +\* Only if the current JavaScript environment supports the reviver with context in the JSON.parse callback. Check compatibility here: + +- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#browser_compatibility + +Otherwise, bigger numbers will lose precision due to the default JavaScript JSON parsing. ## Get Query metadata information @@ -240,3 +247,38 @@ const client = new PrestoClient({ user: 'root', }) ``` + +## Troubleshooting + +### Do not know how to serialize a BigInt + +Example error message: + +``` +Do not know how to serialize a BigInt +TypeError: Do not know how to serialize a BigInt + at JSON.stringify () +``` + +Make sure to write a custom replacer and handle the serialization of BigInts if your Presto query returns a number bigger than `Number.MAX_SAFE_INTEGER`. +Example JSON.stringify replacer: + +```javascript +const results = await client.query(`SELECT 1234567890123456623`) +return { + columns: results.columns, + rows: JSON.stringify(results.data, (key, value) => { + if (typeof value !== 'bigint') return value + + return value.toString() + }), +} +``` + +### Numbers lose precision + +Known issue: +If working with numbers bigger than `Number.MAX_SAFE_INTEGER`, and your environment does not support the `JSON.parse` with the context in the reviver (Node.js > 21.0.0, and certain browser versions), the default JSON.parse will make the number lose precision. +Check compatibility here: + +- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#browser_compatibility From d363ddef8efa7b34ce5038a2e69f244045094f08 Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Fri, 20 Dec 2024 10:50:35 -0600 Subject: [PATCH 6/8] ci(nvm): use latest lts node version --- .nvmrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.nvmrc b/.nvmrc index fb5b513..1d9b783 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -21.0.0 +22.12.0 From 05640654caed56a6d11d30421dcb8d9ad0953542 Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Fri, 20 Dec 2024 10:51:02 -0600 Subject: [PATCH 7/8] refactor(client): minor style changes --- apps/nest-server/src/app/app.service.ts | 2 +- presto-client/src/utils.ts | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/apps/nest-server/src/app/app.service.ts b/apps/nest-server/src/app/app.service.ts index 51ba896..2f4e3bd 100644 --- a/apps/nest-server/src/app/app.service.ts +++ b/apps/nest-server/src/app/app.service.ts @@ -51,7 +51,7 @@ export class AppService { return { columns: results.columns, rows: JSON.stringify(results.data, (key, value) => { - if (typeof value !== 'bigint') return value + if (typeof value != 'bigint') return value return value.toString() }), diff --git a/presto-client/src/utils.ts b/presto-client/src/utils.ts index 5543b81..1a7d7ae 100644 --- a/presto-client/src/utils.ts +++ b/presto-client/src/utils.ts @@ -11,12 +11,10 @@ * @returns Parsed object with BigInts where required */ export function parseWithBigInts(_: string, value: unknown, context: { source: string }) { - if (!context) return value // Context is not available, fallback to default parse - const { source } = context - if (!source) return value // Source is not available, fallback to default parse + if (!context?.source) return value // Context source is not available, fallback to default parse // Ignore non-numbers - if (typeof value !== 'number') return value + if (typeof value != 'number') return value // If not an integer, use the value // TODO: Check if Presto can return floats that could also lose precision @@ -25,5 +23,5 @@ export function parseWithBigInts(_: string, value: unknown, context: { source: s // If number is a safe integer, we can use it if (Number.isSafeInteger(value)) return value - return BigInt(source) + return BigInt(context.source) } From 02686b4a9d63574e7f8974a5f4311b6ded241717 Mon Sep 17 00:00:00 2001 From: Alonso Vega Date: Fri, 20 Dec 2024 14:02:59 -0600 Subject: [PATCH 8/8] test(client): add some initial tests for the utility JSON parser --- presto-client/src/utils.spec.ts | 38 +++++++++++++++++++++++++++++++++ presto-client/src/utils.ts | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 presto-client/src/utils.spec.ts diff --git a/presto-client/src/utils.spec.ts b/presto-client/src/utils.spec.ts new file mode 100644 index 0000000..089221e --- /dev/null +++ b/presto-client/src/utils.spec.ts @@ -0,0 +1,38 @@ +import { parseWithBigInts } from './utils' + +describe('parse big ints', () => { + it('can be plugged into JSON.parse', () => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const primitiveNumber = JSON.parse('123', parseWithBigInts) + expect(primitiveNumber).toBe(123) + + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + const parsedObject = JSON.parse('{"key": "value"}', parseWithBigInts) + expect(parsedObject).toHaveProperty('key') + expect(parsedObject.key).toBe('value') + }) + + it('parses when the JSON.parse reviver context is available', () => { + const parsedValue = parseWithBigInts('key', Number.MAX_SAFE_INTEGER, { + source: Number.MIN_SAFE_INTEGER.toString(), + }) + expect(parsedValue).toBe(Number.MAX_SAFE_INTEGER) + }) + + it('parses when the JSON.parse reviver context is not available', () => { + const parsedValue = parseWithBigInts('key', Number.MAX_SAFE_INTEGER, undefined) + expect(parsedValue).toBe(Number.MAX_SAFE_INTEGER) + }) + + it('parses big integers when JSON.parse reviver context is available', () => { + const largeNumberAsBigInt = BigInt(Number.MAX_SAFE_INTEGER) + 2n + const largeNumberAsNumber = Number.MAX_SAFE_INTEGER + 2 + const parsedValue = parseWithBigInts('key', largeNumberAsNumber, { + source: largeNumberAsBigInt.toString(), + }) + expect(typeof parsedValue).toEqual('bigint') + expect(parsedValue).toEqual(largeNumberAsBigInt) + }) +}) diff --git a/presto-client/src/utils.ts b/presto-client/src/utils.ts index 1a7d7ae..8bb6692 100644 --- a/presto-client/src/utils.ts +++ b/presto-client/src/utils.ts @@ -10,7 +10,7 @@ * @param context Context with source text * @returns Parsed object with BigInts where required */ -export function parseWithBigInts(_: string, value: unknown, context: { source: string }) { +export function parseWithBigInts(_: string, value: unknown, context: { source: string } | undefined) { if (!context?.source) return value // Context source is not available, fallback to default parse // Ignore non-numbers