-
Notifications
You must be signed in to change notification settings - Fork 205
feat: Add high precision TIMESTAMP values for queries #1604
Changes from all commits
35b951b
8bcc181
6842256
74a6639
e556aa0
12be863
f6fe6fb
0dac60d
211fd14
ff7fdec
f3b509c
42c113a
72c7680
711a0b2
00bccf8
ffc419d
6bec4e6
6cedd39
661cc9b
a0f3dee
c8c046c
c893cff
dc37d26
2984d0b
7959fa8
bcfdc40
5e72c89
f150fe9
2e44249
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1099,6 +1099,11 @@ export class BigQuery extends Service { | |
| }; | ||
| }), | ||
| }; | ||
| } else if ((providedType as string).toUpperCase() === 'TIMESTAMP') { | ||
| return { | ||
| type: 'TIMESTAMP', | ||
| timestampPrecision: '12', | ||
| }; | ||
| } | ||
|
|
||
| providedType = (providedType as string).toUpperCase(); | ||
|
|
@@ -1137,7 +1142,10 @@ export class BigQuery extends Service { | |
| } else if (value instanceof BigQueryTime) { | ||
| typeName = 'TIME'; | ||
| } else if (value instanceof BigQueryTimestamp) { | ||
| typeName = 'TIMESTAMP'; | ||
| return { | ||
| type: 'TIMESTAMP', // Was typeName = 'TIMESTAMP', but now we need better timestamp precision; | ||
| timestampPrecision: '12', | ||
| }; | ||
| } else if (value instanceof Buffer) { | ||
| typeName = 'BYTES'; | ||
| } else if (value instanceof Big) { | ||
|
|
@@ -2249,11 +2257,30 @@ export class BigQuery extends Service { | |
| if (res && res.jobComplete) { | ||
| let rows: any = []; | ||
| if (res.schema && res.rows) { | ||
| rows = BigQuery.mergeSchemaWithRows_(res.schema, res.rows, { | ||
| wrapIntegers: options.wrapIntegers || false, | ||
| parseJSON: options.parseJSON, | ||
| }); | ||
| delete res.rows; | ||
| try { | ||
| /* | ||
| Without this try/catch block, calls to getRows will hang indefinitely if | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getRows = jobsQuery |
||
| a call to mergeSchemaWithRows_ fails because the error never makes it to | ||
| the callback. Instead, pass the error to the callback the user provides | ||
| so that the user can see the error. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const listParams = { | ||
| 'formatOptions.timestampOutputFormat': | ||
| queryReq.formatOptions?.timestampOutputFormat, | ||
| 'formatOptions.useInt64Timestamp': | ||
| queryReq.formatOptions?.useInt64Timestamp, | ||
| }; | ||
| rows = BigQuery.mergeSchemaWithRows_(res.schema, res.rows, { | ||
| wrapIntegers: options.wrapIntegers || false, | ||
| parseJSON: options.parseJSON, | ||
| listParams, | ||
| }); | ||
| delete res.rows; | ||
| } catch (e) { | ||
| (callback as SimpleQueryRowsCallback)(e as Error, null, job); | ||
| return; | ||
| } | ||
| } | ||
| this.trace_('[runJobsQuery] job complete'); | ||
| options._cachedRows = rows; | ||
|
|
@@ -2334,6 +2361,18 @@ export class BigQuery extends Service { | |
| if (options.job) { | ||
| return undefined; | ||
| } | ||
| const hasAnyFormatOpts = | ||
| options['formatOptions.timestampOutputFormat'] !== undefined || | ||
| options['formatOptions.useInt64Timestamp'] !== undefined; | ||
| const defaultOpts = hasAnyFormatOpts | ||
| ? {} | ||
| : { | ||
| timestampOutputFormat: 'ISO8601_STRING', | ||
| }; | ||
| const formatOptions = extend(defaultOpts, { | ||
| timestampOutputFormat: options['formatOptions.timestampOutputFormat'], | ||
| useInt64Timestamp: options['formatOptions.useInt64Timestamp'], | ||
| }); | ||
| const req: bigquery.IQueryRequest = { | ||
| useQueryCache: queryObj.useQueryCache, | ||
| labels: queryObj.labels, | ||
|
|
@@ -2342,9 +2381,7 @@ export class BigQuery extends Service { | |
| maximumBytesBilled: queryObj.maximumBytesBilled, | ||
| timeoutMs: options.timeoutMs, | ||
| location: queryObj.location || options.location, | ||
| formatOptions: { | ||
| useInt64Timestamp: true, | ||
| }, | ||
| formatOptions, | ||
| maxResults: queryObj.maxResults || options.maxResults, | ||
| query: queryObj.query, | ||
| useLegacySql: false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -595,10 +595,21 @@ class Job extends Operation { | |
| let rows: any = []; | ||
|
|
||
| if (resp.schema && resp.rows) { | ||
| rows = BigQuery.mergeSchemaWithRows_(resp.schema, resp.rows, { | ||
| wrapIntegers, | ||
| parseJSON, | ||
| }); | ||
| try { | ||
| /* | ||
| Without this try/catch block, calls to getRows will hang indefinitely if | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getRows = jobsQuery endpoint ? |
||
| a call to mergeSchemaWithRows_ fails because the error never makes it to | ||
| the callback. Instead, pass the error to the callback the user provides | ||
| so that the user can see the error. | ||
| */ | ||
| rows = BigQuery.mergeSchemaWithRows_(resp.schema, resp.rows, { | ||
| wrapIntegers, | ||
| parseJSON, | ||
| }); | ||
| } catch (e) { | ||
| callback!(e as Error, null, null, resp); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| let nextQuery: QueryResultsOptions | null = null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| // Copyright 2026 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| import * as assert from 'assert'; | ||
| import {describe, it, before} from 'mocha'; | ||
| import {BigQuery} from '../src'; | ||
|
|
||
| describe.only('High Precision Query System Tests', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need a separated file ? The previous PR already deviated from the pattern of having system tests in the https://github.com/googleapis/nodejs-bigquery/blob/main/system-test/bigquery.ts and added https://github.com/googleapis/nodejs-bigquery/blob/main/system-test/timestamp_output_format.ts, can we at least bundle all timestamp related tests into the same file ? And I'm going to bring again that we don't need the amount of combinations and system tests to check if the support for picosecond resolution is working. More tests doesn't necessarily means more coverage. Just with those two PR we are adding 28 query executions, where previously we had a total of around 50. Not saying that the coverage was perfect before, but adding too many system/integration test just for a time format change. It makes CI slower, and I'm not seeing enough justification for the amount of combination and tests being added here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already got bitten by too much client side validation or tests trying to assert internal details of the service in the past that can cause problems. In this case, is not a SDK code, but is test that can fail in the future if things changes in the service. See internal b/460198628 |
||
| let bigquery: BigQuery; | ||
| const expectedTsValueMicroseconds = '2023-01-01T12:00:00.123456000Z'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was suppose to be Nanosecond |
||
| const expectedTsValueNanoseconds = '2023-01-01T12:00:00.123456789123Z'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and this picosecond. Same applies to the previous https://github.com/googleapis/nodejs-bigquery/pull/1596/changes#diff-e82cb9428a68678cc305e12e59f2e21761622b9fcfcf45d4caab5e80657e2b9a test file |
||
| const expectedErrorMessage = | ||
| 'Cannot specify both timestamp_as_int and timestamp_output_format.'; | ||
|
|
||
| before(() => { | ||
| bigquery = new BigQuery(); | ||
| }); | ||
|
|
||
| const testCases = [ | ||
| { | ||
| name: 'TOF: TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED, UI64: true', | ||
| timestampOutputFormat: 'TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED', | ||
| useInt64Timestamp: true, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED, UI64: false (default ISO8601_STRING)', | ||
| timestampOutputFormat: 'TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED', | ||
| useInt64Timestamp: false, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: FLOAT64, UI64: true (error)', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the service side is going to reject it, we don't need to assert this. If the service side changes behavior (and can happen) we are gonna have a flaky test. |
||
| timestampOutputFormat: 'FLOAT64', | ||
| useInt64Timestamp: true, | ||
| expectedTsValue: undefined, | ||
| expectedError: expectedErrorMessage, | ||
| }, | ||
| { | ||
| name: 'TOF: FLOAT64, UI64: false', | ||
| timestampOutputFormat: 'FLOAT64', | ||
| useInt64Timestamp: false, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: INT64, UI64: true', | ||
| timestampOutputFormat: 'INT64', | ||
| useInt64Timestamp: true, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: INT64, UI64: false (error)', | ||
| timestampOutputFormat: 'INT64', | ||
| useInt64Timestamp: false, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: ISO8601_STRING, UI64: true (error)', | ||
| timestampOutputFormat: 'ISO8601_STRING', | ||
| useInt64Timestamp: true, | ||
| expectedTsValue: undefined, | ||
| expectedError: expectedErrorMessage, | ||
| }, | ||
| { | ||
| name: 'TOF: ISO8601_STRING, UI64: false', | ||
| timestampOutputFormat: 'ISO8601_STRING', | ||
| useInt64Timestamp: false, | ||
| expectedTsValue: expectedTsValueNanoseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: omitted, UI64: omitted (default INT64)', | ||
| timestampOutputFormat: undefined, | ||
| useInt64Timestamp: undefined, | ||
| expectedTsValue: expectedTsValueNanoseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: omitted, UI64: true', | ||
| timestampOutputFormat: undefined, | ||
| useInt64Timestamp: true, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: omitted, UI64: false (default ISO8601_STRING)', | ||
| timestampOutputFormat: undefined, | ||
| useInt64Timestamp: false, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED, UI64: omitted (default INT64)', | ||
| timestampOutputFormat: 'TIMESTAMP_OUTPUT_FORMAT_UNSPECIFIED', | ||
| useInt64Timestamp: undefined, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: FLOAT64, UI64: omitted (error)', | ||
| timestampOutputFormat: 'FLOAT64', | ||
| useInt64Timestamp: undefined, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: INT64, UI64: omitted', | ||
| timestampOutputFormat: 'INT64', | ||
| useInt64Timestamp: undefined, | ||
| expectedTsValue: expectedTsValueMicroseconds, | ||
| }, | ||
| { | ||
| name: 'TOF: ISO8601_STRING, UI64: omitted (error)', | ||
| timestampOutputFormat: 'ISO8601_STRING', | ||
| useInt64Timestamp: undefined, | ||
| expectedTsValue: expectedTsValueNanoseconds, | ||
| }, | ||
| ]; | ||
|
|
||
| testCases.forEach(testCase => { | ||
| it(`should handle ${testCase.name}`, async function () { | ||
| const query = { | ||
| query: 'SELECT ? as ts', | ||
| params: [bigquery.timestamp('2023-01-01T12:00:00.123456789123Z')], | ||
| }; | ||
|
|
||
| const options: any = {}; | ||
| if (testCase.timestampOutputFormat !== undefined) { | ||
| options['formatOptions.timestampOutputFormat'] = | ||
| testCase.timestampOutputFormat; | ||
| } | ||
| if (testCase.useInt64Timestamp !== undefined) { | ||
| options['formatOptions.useInt64Timestamp'] = testCase.useInt64Timestamp; | ||
| } | ||
|
|
||
| try { | ||
| const [rows] = await bigquery.query(query, options); | ||
| if (testCase.expectedError) { | ||
| assert.fail( | ||
| `Query should have failed for ${testCase.name}, but succeeded`, | ||
| ); | ||
| } | ||
| assert.ok(rows.length > 0); | ||
| assert.ok(rows[0].ts.value !== undefined); | ||
| assert.strictEqual(rows[0].ts.value, testCase.expectedTsValue); | ||
| } catch (err: any) { | ||
| if (!testCase.expectedError) { | ||
| throw err; | ||
| } | ||
|
|
||
| const message = err.message; | ||
| assert.strictEqual( | ||
| message, | ||
| testCase.expectedError, | ||
| `Expected ${testCase.expectedError} error for ${testCase.name}, got ${message} (${err.message})`, | ||
| ); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if the comment here is needed