-
Notifications
You must be signed in to change notification settings - Fork 659
fix: Unblock the releases on Node Bigquery #7946
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
Changes from all commits
fd3975c
f31ccc7
5544e36
1bbd192
c588794
73c4ed5
ab16690
2410d66
bdba4dd
ca0a51c
a1eb4d2
a959b3a
f5b7e31
61db403
01ab527
67a4822
757dfdc
cbeeacc
83cfcd8
403093a
83125de
4919e9f
901313b
4fb5ce2
ceaf6c9
c58058b
17593f5
8d6420b
800a818
dc3588f
bd43464
781266f
ee5a084
9e7b452
8707c19
33022c9
e003fa2
0d67c7e
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 |
|---|---|---|
|
|
@@ -55,7 +55,6 @@ import {JobMetadata, JobOptions} from './job'; | |
| import bigquery from './types'; | ||
| import {IntegerTypeCastOptions} from './bigquery'; | ||
| import {RowQueue} from './rowQueue'; | ||
| import IDataFormatOptions = bigquery.IDataFormatOptions; | ||
|
|
||
| // This is supposed to be a @google-cloud/storage `File` type. The storage npm | ||
| // module includes these types, but is current installed as a devDependency. | ||
|
|
@@ -1894,15 +1893,24 @@ class Table extends ServiceObject { | |
| } | ||
| callback!(null, rows, nextQuery, resp); | ||
| }; | ||
|
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. same as https://github.com/googleapis/google-cloud-node/pull/7946/changes#r3029295761, this is the core part that needs to handle the feature flag.
Contributor
Author
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. Ok. For the old version this will now extend the options instead of just setting
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. similar to https://github.com/googleapis/google-cloud-node/pull/7946/changes#r3046889774, code should be along these lines: let qs: GetRowsOptions;
const hasAnyFormatOpts =
options['formatOptions.timestampOutputFormat'] !== undefined ||
options['formatOptions.useInt64Timestamp'] !== undefined;
let defaultOpts = hasAnyFormatOpts
? {}
: {
'formatOptions.useInt64Timestamp': true,
};
if (process.env.BIGQUERY_PICOSECOND_SUPPORT === 'true') {
defaultOpts = hasAnyFormatOpts
? {}
: {
'formatOptions.timestampOutputFormat': 'ISO8601_STRING',
};
}
qs = extend(defaultOpts, options);
Contributor
Author
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. Okay. Made this change. Seems fine. |
||
|
|
||
| const hasAnyFormatOpts = | ||
| options['formatOptions.timestampOutputFormat'] !== undefined || | ||
| options['formatOptions.useInt64Timestamp'] !== undefined; | ||
| const defaultOpts = hasAnyFormatOpts | ||
| let defaultOpts: GetRowsOptions = hasAnyFormatOpts | ||
| ? {} | ||
| : { | ||
| 'formatOptions.timestampOutputFormat': 'ISO8601_STRING', | ||
| 'formatOptions.useInt64Timestamp': true, | ||
| }; | ||
| const qs = extend(defaultOpts, options); | ||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT === 'true') { | ||
| defaultOpts = hasAnyFormatOpts | ||
| ? {} | ||
| : { | ||
| 'formatOptions.timestampOutputFormat': 'ISO8601_STRING', | ||
| }; | ||
| } | ||
| const qs: GetRowsOptions = extend(defaultOpts, options); | ||
|
|
||
| this.request( | ||
| { | ||
| uri: '/data', | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -1042,25 +1042,27 @@ describe('BigQuery', () => { | |||
| }); | ||||
|
|
||||
| it('should create a table with timestampPrecision', async () => { | ||||
| const table = dataset.table(generateName('timestamp-precision-table')); | ||||
| const schema = { | ||||
| fields: [ | ||||
| { | ||||
| name: 'ts_field', | ||||
| type: 'TIMESTAMP', | ||||
| timestampPrecision: 12, | ||||
| }, | ||||
| ], | ||||
| }; | ||||
| try { | ||||
| await table.create({schema}); | ||||
| const [metadata] = await table.getMetadata(); | ||||
| assert.deepStrictEqual( | ||||
| metadata.schema.fields[0].timestampPrecision, | ||||
| '12', | ||||
| ); | ||||
| } catch (e) { | ||||
| assert.ifError(e); | ||||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT === 'true') { | ||||
| const table = dataset.table(generateName('timestamp-precision-table')); | ||||
| const schema = { | ||||
| fields: [ | ||||
| { | ||||
| name: 'ts_field', | ||||
| type: 'TIMESTAMP', | ||||
| timestampPrecision: 12, | ||||
| }, | ||||
| ], | ||||
| }; | ||||
| try { | ||||
| await table.create({schema}); | ||||
| const [metadata] = await table.getMetadata(); | ||||
| assert.deepStrictEqual( | ||||
| metadata.schema.fields[0].timestampPrecision, | ||||
| '12', | ||||
| ); | ||||
| } catch (e) { | ||||
| assert.ifError(e); | ||||
| } | ||||
| } | ||||
| }); | ||||
|
|
||||
|
|
@@ -1562,6 +1564,11 @@ describe('BigQuery', () => { | |||
|
|
||||
| testCases.forEach(testCase => { | ||||
| it(`should handle ${testCase.name}`, async () => { | ||||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT !== 'true') { | ||||
| // These tests are only important when the high precision | ||||
| // timestamp support is turned on. | ||||
| return; | ||||
| } | ||||
|
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. Rather than disabling the tests, we should instead test the feature flag behavior. We should be able to stub the environment variable behavior in tests like this:
Contributor
Author
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. Instead of doing this I modified "system-test:pico": "BIGQUERY_PICOSECOND_SUPPORT=true mocha build/system-test --timeout 600000", so that running system-test script would run all the relevant tests with and without the feature flag even outside of the new tests. The trouble with just enabling the feature flag with these tests is that I think the other team is running these tests with a non-allowlisted project. With a non-allowlisted project and the feature flag turned on these tests will fail and they should fail because they are making high precision timestamp requests to a service that doesn't support it yet. I don't want to create a failing test red herring that will confuse developers who are running this test suite on a non-allowlisted project.
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. which other teams are running these tests ? I think they should have access into the allowlisted project, it was always like that in the BigQuery SDK team. Adding those branches on running all tests makes our CI more complex and I'm not seeing the added value.
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'm with @pearigee recommendation and would add more that I think we should run this whole test suite with and without the flag.
We can have a function runTestsWithFlags(testCase: string, flags: []string, body: () => {}) {
flags.forEach( flag => {
describe(`With {flag}=true`, () => {
beforeEach(() => {
process.env[flag]= 'true'
});
afterEach(() => {
// Clean up to prevent leak
delete process.env[flag];
});
body();
});
describe(`With {flag}=false`, () => {
beforeEach(() => {
process.env[flag]= 'false'
});
afterEach(() => {
// Clean up to prevent leak
delete process.env[flag];
});
body();
});
}
}
const featureFlags = ['BIGQUERY_PICOSECOND_SUPPORT'];
runTestsWithFlags('BigQuery', featureFlags, () => {
/* current test suite */
})
Contributor
Author
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'll get to this one next.
Contributor
Author
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. Alright. I added a high_precision_timestamp.ts file which runs all the other tests again with the I didn't want to use the approach of wrapping bigquery.ts in a loop that runs the tests twice because it would create a massive diff in this PR due to the extra indent. |
||||
| /* | ||||
| The users use the new TIMESTAMP(12) type to indicate they want to | ||||
| opt in to using timestampPrecision=12. The reason is that some queries | ||||
|
|
@@ -1614,6 +1621,11 @@ describe('BigQuery', () => { | |||
| } | ||||
| }); | ||||
| it(`should handle nested ${testCase.name}`, async () => { | ||||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT !== 'true') { | ||||
| // These tests are only important when the high precision | ||||
| // timestamp support is turned on. | ||||
| return; | ||||
| } | ||||
| /* | ||||
| The users use the new TIMESTAMP(12) type to indicate they want to | ||||
| opt in to using timestampPrecision=12. The reason is that some queries | ||||
|
|
@@ -2009,6 +2021,11 @@ describe('BigQuery', () => { | |||
|
|
||||
| testCases.forEach(testCase => { | ||||
| it(`should handle ${testCase.name}`, async () => { | ||||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT !== 'true') { | ||||
| // These tests are only important when the high precision | ||||
| // timestamp support is turned on. | ||||
| return; | ||||
| } | ||||
| /* | ||||
| The users use the new TIMESTAMP(12) type to indicate they want to | ||||
| opt in to using timestampPrecision=12. The reason is that some queries | ||||
|
|
@@ -2063,6 +2080,11 @@ describe('BigQuery', () => { | |||
| } | ||||
| }); | ||||
| it(`should handle nested ${testCase.name}`, async () => { | ||||
| if (process.env.BIGQUERY_PICOSECOND_SUPPORT !== 'true') { | ||||
| // These tests are only important when the high precision | ||||
| // timestamp support is turned on. | ||||
| return; | ||||
| } | ||||
| /* | ||||
| The users use the new TIMESTAMP(12) type to indicate they want to | ||||
| opt in to using timestampPrecision=12. The reason is that some queries | ||||
|
|
||||
|
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. nice solution 👏
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 don't want to block this PR, so this is something we can work on later -- but this goes a bit against convention and our internal style guides. Ideally, we would preserve the relationship between the test file and the file it is testing (i.e. by sharing some portion of the same name). The common pattern for testing the on/off behavior would be something like this: If we were worried about duplication of code, we can always create a utility file that both tests import. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| // 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 {describe, before, after} from 'mocha'; | ||
| import * as path from 'path'; | ||
|
|
||
| // This file registers a second run of the BigQuery system tests with picosecond support enabled. | ||
| // It uses targeted cache clearing to ensure the library and tests are re-initialized with the | ||
| // 'BIGQUERY_PICOSECOND_SUPPORT' environment variable set. | ||
|
|
||
| const originalValue = process.env.BIGQUERY_PICOSECOND_SUPPORT; | ||
|
|
||
| /** | ||
| * Helper to clear the require cache for all files within the current package. | ||
| */ | ||
| function clearPackageCache() { | ||
| // Find the root of the current package by looking for package.json | ||
| const packageJsonPath = require.resolve('../../package.json'); | ||
| const packageDir = path.dirname(packageJsonPath); | ||
|
|
||
| Object.keys(require.cache).forEach(key => { | ||
| if (key.startsWith(packageDir) && !key.includes('node_modules')) { | ||
| delete require.cache[key]; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // 1. REGISTRATION PHASE: | ||
| // Set the environment variable and clear the cache so that the subsequent 'require' | ||
| // of the system test file (and any library files it imports) sees the updated state. | ||
| process.env.BIGQUERY_PICOSECOND_SUPPORT = 'true'; | ||
| clearPackageCache(); | ||
|
|
||
| describe('BigQuery System Tests (High Precision)', () => { | ||
| // 2. EXECUTION PHASE: | ||
| // Mocha runs 'before' hooks before the tests in the required file are executed. | ||
| before(() => { | ||
| process.env.BIGQUERY_PICOSECOND_SUPPORT = 'true'; | ||
| // We don't clear cache here as it's too late for Mocha's registration phase, | ||
| // but we ensure the env var is set for any execution-time checks. | ||
| }); | ||
|
|
||
| after(() => { | ||
| if (originalValue === undefined) { | ||
| delete process.env.BIGQUERY_PICOSECOND_SUPPORT; | ||
| } else { | ||
| process.env.BIGQUERY_PICOSECOND_SUPPORT = originalValue; | ||
| } | ||
| }); | ||
|
|
||
| describe('Run all tests', () => { | ||
| // Wrap all the other tests in a describe block to ensure the entire file is | ||
| // executed when the environment variable is set to true. | ||
|
|
||
| // Programmatically require the tests. Mocha will discover and register them inside this 'describe' block. | ||
| require('./bigquery'); | ||
| require('./timestamp_output_format'); | ||
| }); | ||
| }); | ||
|
|
||
| // 3. CLEANUP: | ||
| // Restore the environment variable after the registration phase to minimize side effects on other test files. | ||
| if (originalValue === undefined) { | ||
| delete process.env.BIGQUERY_PICOSECOND_SUPPORT; | ||
| } else { | ||
| process.env.BIGQUERY_PICOSECOND_SUPPORT = originalValue; | ||
| } |
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.
I think this is the most important part that should handle the feature flag. If BIGQUERY_PICOSECOND_SUPPORT is enabled, we should use
timestampOutputFormat: 'ISO8601_STRING', if not, useuseInt64Timestamp:true. We can still allow customers to pass the timestampOutputFormat, even if is allowlisted now. But in the future they can use it just fine without a new release. Later we can remove the feature flag and just default to use it too.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.
Okay. It looks like we can just change.
to
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.
I think some logic still needs to be shared for both cases. Only the default value should change based on the feature flag.
This code snippets makes it clear that the default should be
useInt64Timestamp: truecurrently, and with the feature flag enabled, should betimestampOutputFormat: 'ISO8601_STRING'. And customers can still provide both opts.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.
Alright. I made this change here and the default request change for getRows. It changes the current defaults that are being used, but it still seems to work okay.