-
Notifications
You must be signed in to change notification settings - Fork 210
feat: support high precision timestamp strings on getRows calls #1596
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
feat: support high precision timestamp strings on getRows calls #1596
Conversation
Add 8 system tests to verify BigQuery `getRows` functionality with various combinations of `timestampOutputFormat` and `useInt64Timestamp`. Tests cover all format options: UNSPECIFIED, FLOAT64, INT64, ISO8601_STRING combined with boolean `useInt64Timestamp` values.
Add 8 system tests to verify BigQuery `getRows` functionality with various combinations of `timestampOutputFormat` and `useInt64Timestamp`. Tests cover all format options: UNSPECIFIED, FLOAT64, INT64, ISO8601_STRING combined with boolean `useInt64Timestamp` values. Asserts that timestamps inserted with picosecond precision are retrieved correctly (truncated to microsecond precision as per BigQuery storage).
to the user
This reverts commit b9e81ac.
Add 7 new system test cases to `system-test/timestamp_output_format.ts` to verify the behavior of `Table.getRows` when `formatOptions.timestampOutputFormat` or `formatOptions.useInt64Timestamp` are omitted. These tests confirm that `useInt64Timestamp` defaults to `true`, causing conflicts with incompatible formats like `FLOAT64` unless explicitly disabled.
This change modifies `BigQueryTimestamp` constructor to preserve original string values for timestamps with more than 9 fractional digits (picoseconds etc.), bypassing `PreciseDate` which truncates to nanoseconds. This ensures that high precision timestamps returned by BigQuery are not truncated by the client. - Unskipped test in `system-test/timestamp_output_format.ts` - Added regression test in `test/bigquery.ts` - Implemented logic to check for high precision strings in `src/bigquery.ts` Co-authored-by: danieljbruce <[email protected]>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
test/table.ts
Outdated
| assert.deepStrictEqual(reqOpts.qs, { | ||
| ...options, | ||
| 'formatOptions.useInt64Timestamp': true, | ||
| 'formatOptions.useInt64Timestamp': false, |
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 ideally, only one of the formatOptions should be send. So in those samples here, just 'formatOptions.timestampOutputFormat': 'ISO8601_STRING', is in the request options. Also instead of so many variations of system tests, I believe it's going to be easier to exercise the different format options via those tests in this file, so we can assert the correct parameter is being send on the request.
The system tests right now are checking the timestamp format and seeing if it has microsecond vs nanosecond resolution, but you can get nanosecond resolution by passing either formatOptions.useInt64Timestamp = true and 'formatOptions.timestampOutputFormat': 'ISO8601_STRING'
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 ideally, only one of the formatOptions should be send
I think my most recent changes meet this requirement.
Also instead of so many variations of system tests, I believe it's going to be easier to exercise the different format options via those tests in this file
I believe the system tests we have now are more valuable. The return value we get when calling getRows is what really matters here. I don't think we should remove the system tests we have right now and replace them with unit tests.
Co-authored-by: Alvaro Viebrantz <[email protected]>
alvarowolfx
left a comment
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.
some question around testing picosecond resolution
| const table = dataset.table(tableId); | ||
| const insertedTsValue = '2023-01-01T12:00:00.123456789123Z'; | ||
| const expectedTsValueMicroseconds = '2023-01-01T12:00:00.123456000Z'; | ||
| const expectedTsValueNanoseconds = '2023-01-01T12:00:00.123456789123Z'; |
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.
can we already test Picosecond resolution or do we need the update from PreciseDate ?
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.
can we already test Picosecond resolution or do we need the update from PreciseDate ?
I'm not sure I understand the question, but I'll try to answer it. It appears that with the current code changes that whenever we specify ISO8601_STRING in the POST request we correctly return 2023-01-01T12:00:00.123456789123Z to the user. That is because of the line of code that says if (value.match(/\.\d{10,}/) && !Number.isNaN(pd.getTime())) {. But when we get Picosecond support we can remove that if block.
Description
This PR achieves the same objectives as googleapis/java-bigquery#4010, but for Node as requested in the planning sheet. The idea is that for reading rows, the users should be able to access high precision values for timestamps if high precision values are being stored on the backend.
Impact
This PR follows a test driven development approach against the getRows method.
Tests were written to evaluate what happens when getRows receives various input values for timestampOutputFormat and useInt64Timestamp. These tests revealed that not only are high precision values not being delivered to users for ISO8601_STRING return types, but also other bugs exist like calls hang on getRows calls that fail and conversion logic throughs errors for some calls that fetch rows. This PR fixes all the bugs and ensures the values with the right precision are delivered to users.
This chart details the before code changes / after code changes results with impact highlighted in green:
The highlighted green impact shows that conversion logic has been updated to avoid the 'cannot convert' errors and with this new logic changes are also applied to the BigQueryTimestamp class to maintain high precision on timestamp values returned to users.
Testing
New system tests to capture all useInt64Timestamp/timestampOutputFormat combinations for getRows calls.