diff --git a/CHANGELOG.md b/CHANGELOG.md index b9f6d533..948d16d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Upgrade `thrift` to 0.25, which depends on the CommonJS-compatible `uuid@11` instead of the ESM-only `uuid@13`, so `require('@databricks/sql')` works again in Jest and other environments without `require(esm)`. (#461) - **Behavior change for direct users of the exported `thrift.TCLIService`** (`DBSQLClient` itself uses the callback form and is unaffected): `TCLIService.Client` calls made without a callback now return native Promises instead of Q promises, and `TCLIService.Processor` runs promise-returning handlers through native Promises. Q-only methods such as `.fail()`, `.fin()`, `.done()` and `.spread()` no longer exist; use `.catch()`, `.finally()` and `.then()`. An unhandled rejection is now a real Node unhandled rejection, which crashes the process under Node's default `--unhandled-rejections=throw`. Q only emitted the `unhandledRejection` event manually, so it never crashed. Handle every promise a callback-less client call returns. - `oauthScopes` is now honored on the Thrift backend: OAuth client-secret M2M sends the configured scopes instead of always `all-apis`, so scoped service-principal secrets can authenticate, and U2M uses them in place of `sql offline_access`. On both backends, entries are split on whitespace and blank values fall back to those defaults. A token read from `persistence` that records different scopes from the ones the client would request is no longer reused or refreshed; the client acquires a new one (for U2M, through a new browser login). Tokens that don't record scopes are still reused. (#540) +- Fix `DECIMAL` parameters losing their fractional digits on the Thrift backend: `"123.45"` was bound as `decimal(3,0)` and came back as `123`. Both backends now send `DECIMAL(p,s)` derived from the value (also for the `DEC` / `NUMERIC` synonyms), stringify `Int64` values beyond 2^53 exactly instead of as `Infinity`, accept exponent forms such as `1e-7` (rewritten as a plain numeral), and throw a `ParameterError` for non-numeric values or values that need more than 38 digits. (#474) ## 3.0.0 diff --git a/lib/DBSQLParameter.ts b/lib/DBSQLParameter.ts index 50a96475..0e77ebd9 100644 --- a/lib/DBSQLParameter.ts +++ b/lib/DBSQLParameter.ts @@ -1,5 +1,6 @@ import Int64 from 'node-int64'; import { TSparkParameter, TSparkParameterValue } from '../thrift/TCLIService_types'; +import ParameterError from './errors/ParameterError'; export type DBSQLParameterValue = undefined | null | boolean | number | bigint | Int64 | Date | string; @@ -60,6 +61,77 @@ function inferNumberType(value: number): DBSQLParameterType { return DBSQLParameterType.DOUBLE; } +const DECIMAL_MAX_PRECISION = 38; + +/** + * Qualify a bare `DECIMAL` (or its `DEC` / `NUMERIC` synonyms) with the + * precision and scale of its value. Both backends otherwise lose the fraction: + * the Thrift server binds a scale-0 decimal, truncating `"123.45"` to `123`, + * and the kernel's raw-param path forwards the type verbatim, so SEA binds it + * as `DECIMAL(10,0)` and rounds. + * + * Precision is significant integer digits plus scale (Spark's decimal-literal + * rule), so `"99.99"` ⇒ `DECIMAL(4,2)` and `"0.00001"` ⇒ `DECIMAL(5,5)`. Plain + * numerals are sent unchanged; exponent forms (`1e-7`, which `Number#toString` + * produces) are rewritten as the equivalent plain numeral. A non-numeric value + * or one needing precision > 38 throws instead of being truncated server-side. + */ +function qualifyDecimal(stringValue: string): { type: string; value: string } { + const m = /^([+-]?)(\d*)(?:\.(\d*))?(?:[eE]([+-]?\d+))?$/.exec(stringValue.trim()); + const [, sign = '', intPart = '', fracPart, exponent] = m ?? []; + if (!m || (intPart === '' && !fracPart)) { + throw new ParameterError( + `DECIMAL parameter value "${stringValue}" is not a decimal number (expected e.g. "123.45" or "1.5e-7").`, + ); + } + + const digits = intPart + (fracPart ?? ''); + const exp = exponent === undefined ? 0 : Number(exponent); + // Position of the decimal point within `digits` once the exponent is applied. + const pointPos = intPart.length + exp; + const leadingZeros = digits.length - digits.replace(/^0+/, '').length; + const isZero = leadingZeros === digits.length; + const scale = Math.max(digits.length - pointPos, 0); + const significantIntDigits = isZero ? 0 : Math.max(pointPos - leadingZeros, 0); + const precision = Math.max(significantIntDigits + scale, 1); + if (precision > DECIMAL_MAX_PRECISION) { + throw new ParameterError( + `DECIMAL parameter value "${stringValue}" needs precision ${precision}, exceeding the Databricks maximum of ` + + `${DECIMAL_MAX_PRECISION}. Round/scale the value or bind it as a STRING.`, + ); + } + + const type = `DECIMAL(${precision},${scale})`; + if (exponent === undefined && fracPart !== '') { + return { type, value: stringValue }; + } + // Rebuild as a plain numeral; both padding lengths are bounded by `precision`. + let plain: string; + if (isZero) { + plain = scale > 0 ? `0.${'0'.repeat(scale)}` : '0'; + } else if (pointPos >= digits.length) { + plain = digits + '0'.repeat(pointPos - digits.length); + } else if (pointPos <= 0) { + plain = `0.${'0'.repeat(-pointPos)}${digits}`; + } else { + plain = `${digits.slice(0, pointPos)}.${digits.slice(pointPos)}`; + } + plain = plain.replace(/^0+(?=\d)/, ''); + return { type, value: sign === '-' ? `-${plain}` : plain }; +} + +// Unqualified decimal type names; a qualified `DECIMAL(p,s)` is left as given. +const BARE_DECIMAL_TYPE = /^(DECIMAL|DEC|NUMERIC)$/i; + +/** + * Exact decimal string for an `Int64`. `Int64#toString` goes through a JS + * number and returns `"Infinity"` for magnitudes ≥ 2^53, so read the + * big-endian two's-complement bytes as a `bigint` instead. + */ +function int64ToString(value: Int64): string { + return value.toBuffer().readBigInt64BE(0).toString(); +} + interface DBSQLParameterOptions { type?: DBSQLParameterType; value: DBSQLParameterValue; @@ -79,7 +151,19 @@ export class DBSQLParameter { this.value = value; } - public toSparkParameter({ name }: ToSparkParameterOptions = {}): TSparkParameter { + public toSparkParameter(options: ToSparkParameterOptions = {}): TSparkParameter { + const param = this.toUnqualifiedSparkParameter(options); + const stringValue = param.value?.stringValue; + // Only an explicit type can be DECIMAL; inferred types never are. + if (this.type !== undefined && BARE_DECIMAL_TYPE.test(this.type.trim()) && typeof stringValue === 'string') { + const decimal = qualifyDecimal(stringValue); + param.type = decimal.type; + param.value = new TSparkParameterValue({ stringValue: decimal.value }); + } + return param; + } + + private toUnqualifiedSparkParameter({ name }: ToSparkParameterOptions): TSparkParameter { // If VOID type was set explicitly - ignore value if (this.type === DBSQLParameterType.VOID) { return new TSparkParameter({ name }); // for NULL neither `type` nor `value` should be set @@ -121,7 +205,7 @@ export class DBSQLParameter { name, type: wireType ?? DBSQLParameterType.BIGINT, value: new TSparkParameterValue({ - stringValue: this.value.toString(), + stringValue: this.value instanceof Int64 ? int64ToString(this.value) : this.value.toString(), }), }); } diff --git a/lib/kernel/KernelPositionalParams.ts b/lib/kernel/KernelPositionalParams.ts index 05db1343..f0dc9d45 100644 --- a/lib/kernel/KernelPositionalParams.ts +++ b/lib/kernel/KernelPositionalParams.ts @@ -13,57 +13,15 @@ // limitations under the License. import { DBSQLParameter, DBSQLParameterValue } from '../DBSQLParameter'; -import ParameterError from '../errors/ParameterError'; import { KernelNativeRawParameterInput } from './KernelNativeLoader'; import assertBindableValue from '../utils/assertBindableValue'; -/** - * Derive `(precision,scale)` from a decimal value string for the kernel - * `DECIMAL(p,s)` type name — the kernel param codec requires the parenthesised - * form (plain `"DECIMAL"` is rejected) so it preserves the caller's fractional - * digits. `"99.99"` ⇒ `"4,2"`; `"-123"` ⇒ `"3,0"`; `"0.00001"` ⇒ `"5,5"`. - * - * Precision is significant integer digits (insignificant leading zeros - * stripped, matching Spark's decimal-literal rule) plus the fractional scale. - * The value is NOT clamped: a non-numeric value, or one that needs precision > - * the Databricks maximum of 38, throws `ParameterError` at bind time rather - * than emitting an inconsistent `DECIMAL(38,…)` type alongside an - * unrepresentable value (which the kernel would reject or silently truncate). - */ -function decimalPrecisionScale(v: string): string { - // Accept an optional sign + plain decimal numeral only. Exponential form - // ("1e+21"), empty, and non-numeric ("abc") are rejected with a clear error - // instead of mis-deriving (p,s) and surfacing an opaque kernel failure. - const m = /^[+-]?(\d*)(?:\.(\d+))?$/.exec(v.trim()); - const intPart = m?.[1] ?? ''; - const fracPart = m?.[2] ?? ''; - if (!m || (intPart === '' && fracPart === '')) { - throw new ParameterError( - `DECIMAL parameter value "${v}" is not a plain decimal numeral (expected [+-]?digits[.digits]).`, - ); - } - const scale = fracPart.length; - // Significant integer digits: drop insignificant leading zeros ("007" → 1, - // "0" / "" → 0) so e.g. 0.00001 is DECIMAL(5,5), not DECIMAL(6,5). - const significantIntDigits = intPart.replace(/^0+/, '').length; - const precision = Math.max(significantIntDigits + scale, 1); - if (precision > 38) { - throw new ParameterError( - `DECIMAL parameter value "${v}" needs precision ${precision}, exceeding the Databricks maximum of 38. ` + - 'Round/scale the value or bind it as a STRING.', - ); - } - return `${precision},${scale}`; -} - /** * Reduce a `DBSQLParameter | DBSQLParameterValue` to the napi * `RawParameterInput` (`{ name?, sqlType, value? }`) accepted by the kernel's * raw-parameter path. Reuses `DBSQLParameter.toSparkParameter` — the same - * type-inference + value-stringification the Thrift backend uses — then adapts - * the type name where required: - * - DECIMAL → `DECIMAL(p,s)` (parenthesised form required) - * - a missing value ⇒ SQL NULL. + * type-inference + value-stringification (including DECIMAL → `DECIMAL(p,s)`) + * the Thrift backend uses; a missing value ⇒ SQL NULL. */ function toRawParameterInput(value: DBSQLParameter | DBSQLParameterValue): KernelNativeRawParameterInput { const param = value instanceof DBSQLParameter ? value : new DBSQLParameter({ value }); @@ -76,12 +34,7 @@ function toRawParameterInput(value: DBSQLParameter | DBSQLParameterValue): Kerne return { sqlType: 'VOID' }; } - let sqlType = spark.type ?? 'STRING'; - const upper = sqlType.toUpperCase(); - if (upper === 'DECIMAL') { - sqlType = `DECIMAL(${decimalPrecisionScale(stringValue)})`; - } - return { sqlType, value: stringValue }; + return { sqlType: spark.type ?? 'STRING', value: stringValue }; } /** diff --git a/tests/e2e/kernel/execution-e2e.test.ts b/tests/e2e/kernel/execution-e2e.test.ts index 772a53a5..ced945e7 100644 --- a/tests/e2e/kernel/execution-e2e.test.ts +++ b/tests/e2e/kernel/execution-e2e.test.ts @@ -147,6 +147,34 @@ describe('kernel execution end-to-end', function e2eSuite() { } }); + it('binds DECIMAL parameters with value-derived precision and scale', async () => { + const client = new DBSQLClient(); + + await client.connect({ + host: hostName as string, + path: httpPath as string, + token: token as string, + useKernel: true, + } as ConnectionOptions & InternalConnectionOptions); + + const session = await client.openSession({ initialCatalog: 'main' }); + let operation; + try { + const decimal = (value: string | number) => new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value }); + operation = await session.executeStatement('SELECT ? AS a, typeof(?) AS a_type, ? AS b, typeof(?) AS b_type', { + // `1e-7` exercises the exponent form `Number#toString` produces. + ordinalParameters: [decimal('123.45'), decimal('123.45'), decimal(1e-7), decimal(1e-7)], + }); + expect(await operation.fetchAll()).to.deep.equal([ + { a: 123.45, a_type: 'decimal(5,2)', b: 1e-7, b_type: 'decimal(7,7)' }, + ]); + } finally { + await operation?.close(); + await session.close(); + await client.close(); + } + }); + it('binds named NULL and empty string through rawParams on the async path', async () => { const client = new DBSQLClient(); diff --git a/tests/e2e/query_parameters.test.ts b/tests/e2e/query_parameters.test.ts index d6d47b6b..65195122 100644 --- a/tests/e2e/query_parameters.test.ts +++ b/tests/e2e/query_parameters.test.ts @@ -232,6 +232,48 @@ describe('Query parameters', () => { } }); + // https://github.com/databricks/databricks-sql-nodejs/issues/474 + it('should keep the precision and scale of DECIMAL parameters', async () => { + const session = await openSession(); + try { + const operation = await session.executeStatement( + 'SELECT :a AS a, typeof(:a) AS a_type, :b AS b, typeof(:b) AS b_type, :c AS c, typeof(:c) AS c_type', + { + namedParameters: { + a: new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: '123.45' }), + // `Number#toString` renders this as "-1.5e-7". + b: new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: -1.5e-7 }), + // More digits than the scale-0 DECIMAL the server used to apply could hold. + c: new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: BigInt('12345678901234') }), + }, + }, + ); + const result = await operation.fetchAll(); + await operation.close(); + expect(result).to.deep.equal([ + { + a: 123.45, + a_type: 'decimal(5,2)', + b: -1.5e-7, + b_type: 'decimal(8,8)', + c: 12345678901234, + c_type: 'decimal(14,0)', + }, + ]); + + const ordinal = await session.executeStatement('SELECT ? AS v, typeof(?) AS t', { + ordinalParameters: [ + new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: '123.45' }), + new DBSQLParameter({ type: DBSQLParameterType.DECIMAL, value: '123.45' }), + ], + }); + expect(await ordinal.fetchAll()).to.deep.equal([{ v: 123.45, t: 'decimal(5,2)' }]); + await ordinal.close(); + } finally { + await session.close(); + } + }); + // https://github.com/databricks/databricks-sql-nodejs/issues/261 describe('non-scalar parameter values (issue #261)', () => { // Non-scalar values are rejected client-side (before any RPC is sent), so executeStatement diff --git a/tests/unit/DBSQLParameter.test.ts b/tests/unit/DBSQLParameter.test.ts index d407f2bd..4070ad8d 100644 --- a/tests/unit/DBSQLParameter.test.ts +++ b/tests/unit/DBSQLParameter.test.ts @@ -2,6 +2,7 @@ import { expect } from 'chai'; import Int64 from 'node-int64'; import { TSparkParameterValue, TSparkParameter } from '../../thrift/TCLIService_types'; import { DBSQLParameter, DBSQLParameterType, DBSQLParameterValue } from '../../lib/DBSQLParameter'; +import ParameterError from '../../lib/errors/ParameterError'; describe('DBSQLParameter', () => { it('should infer types correctly', () => { @@ -169,6 +170,104 @@ describe('DBSQLParameter', () => { ); }); + describe('DECIMAL', () => { + const decimal = (value: DBSQLParameterValue, type: string = DBSQLParameterType.DECIMAL) => + new DBSQLParameter({ type: type as DBSQLParameterType, value }).toSparkParameter(); + const expected = (type: string, stringValue: string) => + new TSparkParameter({ type, value: new TSparkParameterValue({ stringValue }) }); + + it('derives precision and scale from the value instead of sending a scale-0 DECIMAL', () => { + // A bare `DECIMAL` makes the Thrift server bind decimal(p,0): "123.45" → 123. + expect(decimal('123.45')).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal('-123.45')).to.deep.equal(expected('DECIMAL(5,2)', '-123.45')); + expect(decimal('-123')).to.deep.equal(expected('DECIMAL(3,0)', '-123')); + expect(decimal('1.500')).to.deep.equal(expected('DECIMAL(4,3)', '1.500')); + expect(decimal('99.99', 'decimal')).to.deep.equal(expected('DECIMAL(4,2)', '99.99')); + }); + + it('excludes insignificant leading zeros from precision', () => { + expect(decimal('0.00001')).to.deep.equal(expected('DECIMAL(5,5)', '0.00001')); + expect(decimal('007.50')).to.deep.equal(expected('DECIMAL(3,2)', '007.50')); + expect(decimal('.5')).to.deep.equal(expected('DECIMAL(1,1)', '.5')); + expect(decimal('0')).to.deep.equal(expected('DECIMAL(1,0)', '0')); + expect(decimal('000.000')).to.deep.equal(expected('DECIMAL(3,3)', '000.000')); + }); + + it('derives precision and scale for number, bigint and Int64 values', () => { + expect(decimal(123.45)).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal(-7)).to.deep.equal(expected('DECIMAL(1,0)', '-7')); + expect(decimal(BigInt('12345678901234567890'))).to.deep.equal(expected('DECIMAL(20,0)', '12345678901234567890')); + expect(decimal(new Int64(1234))).to.deep.equal(expected('DECIMAL(4,0)', '1234')); + }); + + it('stringifies Int64 values beyond 2^53 exactly', () => { + // `Int64#toString` returns "Infinity" from 2^53 upwards. + expect(decimal(new Int64(0x200000, 0))).to.deep.equal(expected('DECIMAL(16,0)', '9007199254740992')); + expect(decimal(new Int64(0x200000, 1))).to.deep.equal(expected('DECIMAL(16,0)', '9007199254740993')); + expect(decimal(new Int64(0x7fffffff, 0xffffffff))).to.deep.equal( + expected('DECIMAL(19,0)', '9223372036854775807'), + ); + expect(decimal(new Int64(0x80000000, 0))).to.deep.equal(expected('DECIMAL(19,0)', '-9223372036854775808')); + expect(decimal(new Int64(-5))).to.deep.equal(expected('DECIMAL(1,0)', '-5')); + expect(new DBSQLParameter({ value: new Int64(0x200000, 1) }).toSparkParameter()).to.deep.equal( + expected('BIGINT', '9007199254740993'), + ); + }); + + it('qualifies the DEC and NUMERIC synonyms like DECIMAL', () => { + expect(decimal('123.45', 'NUMERIC')).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal('123.45', 'numeric')).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal('123.45', 'DEC')).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal('123.45', ' Decimal ')).to.deep.equal(expected('DECIMAL(5,2)', '123.45')); + expect(decimal('123.45', 'NUMERIC(10,2)')).to.deep.equal(expected('NUMERIC(10,2)', '123.45')); + expect(decimal('123.45', 'DECIMALS')).to.deep.equal(expected('DECIMALS', '123.45')); + }); + + it('rewrites exponent forms as plain numerals', () => { + // `Number#toString` uses exponent notation below 1e-6 and from 1e21. + expect(decimal(1e-7)).to.deep.equal(expected('DECIMAL(7,7)', '0.0000001')); + expect(decimal(-1.5e-7)).to.deep.equal(expected('DECIMAL(8,8)', '-0.00000015')); + expect(decimal(1e21)).to.deep.equal(expected('DECIMAL(22,0)', '1000000000000000000000')); + expect(decimal('1.5e3')).to.deep.equal(expected('DECIMAL(4,0)', '1500')); + expect(decimal('1.50E1')).to.deep.equal(expected('DECIMAL(3,1)', '15.0')); + expect(decimal('+12.5e-1')).to.deep.equal(expected('DECIMAL(3,2)', '1.25')); + expect(decimal('0.0e5')).to.deep.equal(expected('DECIMAL(1,0)', '0')); + expect(decimal('0e999999999999')).to.deep.equal(expected('DECIMAL(1,0)', '0')); + expect(decimal('123.')).to.deep.equal(expected('DECIMAL(3,0)', '123')); + }); + + it('accepts the full 38-digit precision', () => { + const value = '12345678901234567890.123456789012345678'; + expect(decimal(value)).to.deep.equal(expected('DECIMAL(38,18)', value)); + expect(decimal('-0.12345678901234567890123456789012345678')).to.deep.equal( + expected('DECIMAL(38,38)', '-0.12345678901234567890123456789012345678'), + ); + }); + + it('rejects a value that needs precision > 38 instead of truncating it', () => { + const tooBig = () => decimal('123456789012345678901234567890123456789'); + expect(tooBig).to.throw(ParameterError, /needs precision 39, exceeding the Databricks maximum of 38/); + expect(() => decimal('0.000000000000000000000000000000000000001')).to.throw(ParameterError, /precision 39/); + expect(() => decimal('1e38')).to.throw(ParameterError, /precision 39/); + expect(() => decimal('1e-39')).to.throw(ParameterError, /precision 39/); + expect(() => decimal(`1e${'9'.repeat(400)}`)).to.throw(ParameterError, /exceeding the Databricks maximum/); + }); + + it('rejects non-numeric values', () => { + for (const value of ['abc', '', ' ', '.', 'e5', '1,000', '1e', '0x10', 'NaN', NaN, Infinity, true]) { + expect(() => decimal(value), String(value)).to.throw(ParameterError, /is not a decimal number/); + } + }); + + it('leaves an explicitly qualified DECIMAL(p,s) type unchanged', () => { + expect(decimal('123.45', 'DECIMAL(10,2)')).to.deep.equal(expected('DECIMAL(10,2)', '123.45')); + }); + + it('still binds a NULL DECIMAL without a type', () => { + expect(decimal(null)).to.deep.equal(new TSparkParameter({})); + }); + }); + it('binds a typed DATE from local calendar accessors, never via toISOString (no UTC off-by-one)', () => { // A `Date` constructed from local components must bind the wall-calendar // date the user intended, regardless of the process timezone. diff --git a/tests/unit/kernel/positionalParams.test.ts b/tests/unit/kernel/positionalParams.test.ts index 4e4e4009..b191c2ab 100644 --- a/tests/unit/kernel/positionalParams.test.ts +++ b/tests/unit/kernel/positionalParams.test.ts @@ -57,10 +57,14 @@ describe('KernelPositionalParams.buildKernelPositionalParams', () => { ); }); - it('rejects a non-numeric / exponential DECIMAL value', () => { - expect(decimal('1e+21')).to.throw(ParameterError, /not a plain decimal numeral/); - expect(decimal('abc')).to.throw(ParameterError, /not a plain decimal numeral/); - expect(decimal('')).to.throw(ParameterError, /not a plain decimal numeral/); + it('rewrites an exponent-form DECIMAL value as a plain numeral', () => { + expect(decimal('1e+21')()).to.deep.equal([{ sqlType: 'DECIMAL(22,0)', value: '1000000000000000000000' }]); + expect(decimal('1e-7')()).to.deep.equal([{ sqlType: 'DECIMAL(7,7)', value: '0.0000001' }]); + }); + + it('rejects a non-numeric DECIMAL value', () => { + expect(decimal('abc')).to.throw(ParameterError, /is not a decimal number/); + expect(decimal('')).to.throw(ParameterError, /is not a decimal number/); }); it('preserves qualified INTERVAL types for the kernel raw binder', () => {