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

feat(BigQuery): Add JSON datatype #6412

Merged
merged 5 commits into from
Aug 4, 2023
Merged

feat(BigQuery): Add JSON datatype #6412

merged 5 commits into from
Aug 4, 2023

Conversation

ajupazhamayil
Copy link
Contributor

@ajupazhamayil ajupazhamayil commented Jul 4, 2023

This PR add JSON datatype in BigQuery. Also resolves #6413 #5151

go/php-bigquery-json-datatype

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the BigQuery API. label Jul 4, 2023
@ajupazhamayil ajupazhamayil force-pushed the jsonBigQuery branch 7 times, most recently from 7d0931b to e33ee4d Compare July 9, 2023 20:10
@ajupazhamayil ajupazhamayil marked this pull request as ready for review July 10, 2023 08:30
@ajupazhamayil ajupazhamayil requested review from a team as code owners July 10, 2023 08:30
Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Approach looks good, but make tests more robust.

@ajupazhamayil
Copy link
Contributor Author

@vishwarajanand Made the required changes, Please have a look! Thank you!

Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Per our internal discussions, we want the user to parse the JSON and the library will only provide a JSON string.
Some considerations (not all) are PHP's json_encode or json_decode doesn't actually cover all the possible range of JSON values (such as float, decimals) as supported by backend, nested JSON depth also might increase in future, as opposed to max 512 allowed in PHP.

Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Change the design to accept and return JSON as strings

Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Left a few questions which potentially requires some changes.

Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some of BigQuery system tests are failing
2 participants