Skip to content
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

Added support for the NUMERIC values. #119

Merged
merged 6 commits into from
Jun 22, 2018

Conversation

aprgoog
Copy link
Contributor

@aprgoog aprgoog commented Jun 12, 2018

This change adds support for the newly added NUMERIC data type to the nodejs BigQuery library.

@codecov
Copy link

codecov bot commented Jun 12, 2018

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #119   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         696    704    +8     
=====================================
+ Hits          696    704    +8
Impacted Files Coverage Δ
src/table.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c40d277...f5cbdf2. Read the comment docs.

Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

I'm not familiar enough with the Node.js ecosystem to know if big.js is the right choice, but it does appear to have some healthy activity on the repo. https://github.com/MikeMcl/big.js

Note to others reviewing: this PR was authored by a Googler on the BigQuery team.

@JustinBeckwith
Copy link
Contributor

Good question for @googleapis/node-team :)

@ofrobots
Copy link

What's the forward compatibility story with the core BigInt language feature that we just shipped with Node 10?

Copy link

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking until forward compatibility story is understood.

@tswast
Copy link
Contributor

tswast commented Jun 13, 2018

@ofrobots Do BigInts support fractional values?

@ofrobots ofrobots dismissed their stale review June 13, 2018 00:30

Fractional values needed.

@ofrobots
Copy link

It does not. I've removed the block. Will take a deeper look later.

@ofrobots
Copy link

For posterity here is a link to the BigQuery docs on the NUMERIC data type: https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#numeric-type

Copy link

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the choice of big.js. It seems to be a fairly healthy project with a large amount of usage. There also seems to be a corresponding types package which itself is fairly heavily used. Seems like a good choice.

@stephenplusplus to approve.

@stephenplusplus
Copy link
Contributor

LGTM, but we should probably have a system test for this new type.

@aprgoog
Copy link
Contributor Author

aprgoog commented Jun 21, 2018

@stephenplusplus Added a system test, please take another look.

Reverted a change that the local linter made but the CI's one
does not like.
@aprgoog
Copy link
Contributor Author

aprgoog commented Jun 22, 2018

Can somebody merge the changes? I do not have write access to the repo.

@stephenplusplus stephenplusplus merged commit 32b7bdd into googleapis:master Jun 22, 2018
@witbybit
Copy link

witbybit commented Jan 31, 2019

I upgraded the bigquery node js library after a long time and my app broke because of numeric values being returned as Big() values. Is it possible for me to have the existing behaviour where I got the numeric result back as string? I am storing the query result in firestore which won't support Big() and converting the results manually to string seems tedious. Is there any query option one can use to disable this new behaviour?

@ElliottBrossard
Copy link

ElliottBrossard commented Jan 31, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants