search

lquerel / gcp-bigquery-client

GCP BigQuery Client (Rust)
Apache License 2.0
92 stars 60 forks source link

Adding Json to FieldType and missing helper methods #62

Closed mchlgibs closed 1 year ago

mchlgibs commented 1 year ago

Adding Json as a field type

Complete list is here: https://cloud.google.com/bigquery/docs/reference/rest/v2/tables#TableFieldSchema

Json seems to be the only one that was missing in the enum, but Json & Geography were both missing in the helper functions.

This should fix https://github.com/lquerel/gcp-bigquery-client/issues/56

lquerel commented 1 year ago

First, thanks for this contribution.

This contribution addresses the schema aspect of JSON support, but have you verified that this is sufficient for query-side JSON support? In other words, have you checked if you can query a table containing a JSON column? It would be beneficial to update the existing unit tests to include this validation.

mchlgibs commented 1 year ago

I haven't checked. I'll look at it over the weekend and see what I can do about adding tests. (And code if necessary.)

mchlgibs commented 1 year ago

Checked and added a test.

mchlgibs commented 1 year ago

@lquerel it's ready for you to inspect and merge.

lquerel commented 1 year ago

Sorry for the delay.

In your example the Rust type of the json column is String but in my opinion it will be more natural to use serde_json::value::Value in order to deserialize the json value in a dynamic structure directly navigable.

mchlgibs commented 1 year ago

I added a third commit, where I change the type of the json column to serde_json::value::Value. There was kind of a trick though - when you serialize it into the request - you have to serialize that column as a single string, rather than as a JSON object. So I implemented a function to do that and updated the test. Assuming we add this functionality, other users will have to annotate their structs similarly.

I'd appreciate advice on the following: 1) Is the helper in the right place, or is there a more logical place to put it? 2) Is this even the right way to go?

mchlgibs commented 1 year ago

@lquerel - all done :)

Assuming you like this, I'd recommend a squash, rather than just a merge.

lquerel commented 1 year ago

@lquerel - all done :)

Assuming you like this, I'd recommend a squash, rather than just a merge.

I will work on this over the weekend. Thanks

lquerel commented 1 year ago

@mchlgibs merged and published. Thank you.

mchlgibs commented 1 year ago

Thank you so much.