search

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

fix: BigQueryTimestamp should keep accepting floats #1339

Closed alvarowolfx closed 8 months ago

alvarowolfx commented 8 months ago

Add back parsing of timestamps as floats, as some users where relying on that internal format.

A example of that is passing Date.now()/1000). This worked in the past because internally the library was parsing BigQuery timestamps being returned as a float representation from the service side. We moved away from it due to it being imprecise and now the formatOptions.useInt64Timestamp is available, which makes the timestamps to be return as a int64 in usec.

This PR restores the BigQueryTimestamp parsing and converts the int64 timestamp coming from the service side before passing to the BigQueryTimestamp constructor, so the class don't need to handle microsecond parsing for now and we keep it backward compatible.

Fixes #1338 🦕

BEGIN_COMMIT_OVERRIDE fix: BigQueryTimestamp should keep accepting floats #1339

fix: restores BigQueryTimestamp behavior to accept a numeric value in the constructor representing epoch-seconds. The affected 7.5.0 version would parse a numeric value as epoch-microseconds.

fix: add better documentation around usage of BigQueryTimestamp class and .timestamp method. END_COMMIT_OVERRIDE

yeoffrey commented 8 months ago

As you mentioned in #1338 if you want to move away from accepting floats, you could mark this as depreciated or update the documentation for this class to mention it. I think I would've opted to work with Date if it was mentioned in the docs or in the code comments as a preferred way. Thanks for the PR 👏

alvarowolfx commented 8 months ago

As you mentioned in #1338 if you want to move away from accepting floats, you could mark this as depreciated or update the documentation for this class to mention it. I think I would've opted to work with Date if it was mentioned in the docs or in the code comments as a preferred way. Thanks for the PR 👏

But our public docs only mention the BigQueryTimestamp class being used with a Date type and the method signature doesn't specify how the number param should be informed, so you ended up using the undocumented internal float without it being mentioned that it can be used.

I checking here, but I think we don't have a way to mark the v7.5.0 as having a possible breaking change and also I can't deprecate the usage of floats as this was already undocumented.

I opened this PR to evaluate how would be to add float parsing back to avoid a possible breaking change, but the solution is not perfect and can cause some issues depending on how is used by a customer. So I'm leaning towards not merging this PR.

stefandoorn commented 8 months ago

@alvarowolfx Does it make sense to revert the previous commit and tag 7.5.1 as a fix for that, then re-implement the change and tag 8.0.0 as a breaking change? I'm not sure though what is the Google policy on tagging majors.

static timestamp(value: Date | PreciseDate | string | number) {

This does mention the usage of number which implies a numeric timestamp, although it's slightly unclear indeed what needs to go in there. We have been using this for ages, so I'm not sure how we got to this way of working in the first place, and the docs from then probably don't exist anymore.

Given the fact it's undocumented I do understand your standpoint though. Purely technical it might be a BC, but you could also reason it's a bugfix and therefore not BC at all.

If none of these are possible, maybe you can adjust the 7.5.0 release note to mention this, so it's easier for others to understand any issue they might encounter.

alvarowolfx commented 8 months ago

@stefandoorn these would be the exact steps to turn this into a breaking change and our release tooling would bump the package to 8.0.0 automatically.

Does it make sense to revert the previous commit and tag 7.5.1 as a fix for that, then re-implement the change and tag 8.0.0 as a breaking change? I'm not sure though what is the Google policy on tagging majors.

What I'm considering here is if this should be classified as a breaking change or not. Of course I ended up causing a major headache for you guys, so that alone might be a reason for it, but I wonder who else was using that internal float representation by mistake. I'll discuss this with my team today and have a final answer by EOD

alvarowolfx commented 8 months ago

Just to add more information here, the optional number type as input was added an year ago to the BigQueryTimestamp and BigQuery.timestamp methods, on this PR https://github.com/googleapis/nodejs-bigquery/pull/1192, which added support for PreciseDate class and microseconds precision. This was added on v6.2.0. So it's a relative recent addition and not documented on how that number should be passed. From what was reported, you were using this number input even before that.

stefandoorn commented 8 months ago

Hmmm, then I must be mixing up some things here.

I think we've been passing the numeric timestamp around always between our services, but only more recent started drafting our own BigQueryTimestamp as that looked more in line with the BQ package.

alvarowolfx commented 8 months ago

Just to clarify @stefandoorn, the float parsing is there for years like you mentioned, but the signature was only changed to add number type as input on the previous mentioned PR and published on v6.2.0. So you could be passing as a number for a while and if you are not using Typescript, you just thought that was fine and accepting the number input.

Hmmm, then I must be mixing up some things here.

I think we've been passing the numeric timestamp around always between our services, but only more recent started drafting our own BigQueryTimestamp as that looked more in line with the BQ package.

yeoffrey commented 8 months ago

Hey @shollyman 👋, as a user of this package it was a bit of a guessing game as to what the number represented, so I think that would be a great idea to update the docs to make sure thats clear. When I originally tried to find the information on this class I was looking at the BigQuery Node Reference page for it, but didn't find much info.

alvarowolfx commented 8 months ago

Generally looks good. Should we be clearer in the BigQueryTimestamp documentation about what the numeric format represents? It may help users avoid problems if they do use a number value in the constructor.

yes, gonna add extra docs about that.

alvarowolfx commented 8 months ago

@yeoffrey @stefandoorn I added more comments to the timestamp related functions and also the good news is that we are bringing back the parsing functionality of the BigQueryTimestamp class and still making it work with the newer changes for better and more precise int64 timestamps. So this will be a fix to the previous breaking change and will keep it backward compatible.

stefandoorn commented 8 months ago

Thanks @alvarowolfx - appreciate the quick handling of this! :)

yeoffrey commented 8 months ago

Thank you very much!