googleapis / nodejs-bigquery

Node.js client for Google Cloud BigQuery: A fast, economical and fully-managed enterprise data warehouse for large-scale data analytics.
https://cloud.google.com/bigquery/
Apache License 2.0
467 stars 211 forks source link

7.5.0 is a breaking change #1338

Closed yeoffrey closed 8 months ago

yeoffrey commented 8 months ago

Issue Summary:

The latest version (7.5.0) of the Node.js Google BigQuery Client Library has introduced a breaking change in the timestamp representation. The library has switched from the previous representation, where timestamps could be created as numbers using new BigQueryTimestamp(Date.now() / 1000), to a new representation using int64 microseconds. This change results in incorrect dates when creating BigQueryTimestamps using the previous method, and now requires multiplying the timestamp by 1000 to obtain the correct date.

Steps to Reproduce:

Install or update the library to version 7.4.0. Attempt to create a BigQueryTimestamp using the old method: new BigQueryTimestamp(Date.now() / 1000). Then update to 7.5.0 Attempt to create a BigQueryTimestamp using the old method: new BigQueryTimestamp(Date.now() / 1000).

Expected Behavior:

The expected behavior is that creating a BigQueryTimestamp using new BigQueryTimestamp(Date.now() / 1000) should result in the correct date representation.

Actual Behavior:

The actual behavior is that the timestamp representation has changed, and creating a BigQueryTimestamp using new BigQueryTimestamp(Date.now() / 1000) now produces an entirely different date in 1970. The correct date is only obtained when using new BigQueryTimestamp(Date.now() * 1000).

Impact:

This is a breaking change as it affects existing code that relies on the previous timestamp representation. Applications using the library may experience incorrect date values, leading to potential data inconsistencies. In our projects our timestamps are now being represented incorrectly, and because this wasn't marked we need to make some hot fixes.

Proposed Solution:

Considering the breaking nature of this change and following semantic versioning, I think this should've been marked as a Major update to communicate the breaking nature of the timestamp representation switch. Additionally, providing clear documentation and migration steps for users upgrading to version 7.5.0 or later would help minimize the impact on existing projects.

Additional Information:

Node.js version: 20 Operating System: MacOs

alvarowolfx commented 8 months ago

Sorry for that issue @yeoffrey, I totally missed the case where users were instantiating the BigQueryTimestamp object using a number in milliseconds and considered that to be only used internally. The tests cases that we had for external usage was only for Date objects and datetime strings. I'll work on a fix for that, either by allowing it to accept milliseconds too or just rollback the behavior for now. And sorry for the delay on answering this important issue, I was out last week.

yeoffrey commented 8 months ago

All good @alvarowolfx and thanks for the info. Please let me know if you need some additional information or testing help :) Just to clarify, Date.now() returns a timestamp in milliseconds and before 7.5.0 we would pass in seconds, which is why I divided by 1000 in the example šŸ‘

alvarowolfx commented 8 months ago

Oh, that's a interesting point @yeoffrey . Without knowing, you were relying on the float representation that BigQuery was using before and we were trying to more away from it (that format has some precision problems). Doing the Date.now/1000 was resulting in a float number and executing that code path:

I'm actively investigating here how can we keep the old behavior and move to the newer lossless int64 usec representation.

yeoffrey commented 8 months ago

Interesting! Glad to see why that was working then. We're internally trying to move over to Date so we don't have to do deal with this situation in the future. If you find it doesn't make sense to support this implementation, its also fine to remove support for it but in that case I would mark it as a major upgrade to bring some attention to this change. Thanks for your explanation!