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

(#1626) fix for RPC error with BQ nested fields #1638

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Jul 30, 2019

First cut here - wanted to open this up for review.

In #1626, some issues with passing a list of dicts to agate.Table.from_object are outlined. This PR changes the BigQuery plugin to bypass from_object, instead creating an object explicitly from a specified set of rows and columns. This has the very negative effect of requiring us to serialize iterables as json (if we didn't do this manually, Agate would just use their reprs which is pretty unhelpful) -- see the included integration tests for an example.

I think it may be appropriate to convert these nested/repeated records from dicts/lists (respectively) into json strings. Previously, dbt would fail with an error if any columns returned by adapter.execute were nested or repeated, so just returning without error is a marked improvement over the existing behavior.

I'm not sure if there's a good way to reconcile Agate's behavior with BigQuery's nested/repeated records. Depending on how we choose to proceed, this issue either represents another nail in Agate's coffin for dbt, or a tacit decision to not leverage nested/repeated records on BigQuery. My personal stance is that we should ditch Agate as soon as we're able.

I believe BQ is the only place where a call to execute can return data that isn't strictly a list of dicts that map strings onto scalars. We can use table_from_data_flat in other adapters to bypass the type inference that Agate does -- that might be a good idea as well, though possibly one for another PR.

@drewbanin drewbanin requested a review from beckjake July 30, 2019 13:51
Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

This seems fine, given that we're tied to agate for at least the moment. Obviously we're really jamming a square peg into a round hole here.

core/dbt/clients/agate_helper.py Outdated Show resolved Hide resolved
@drewbanin drewbanin merged commit 7177a65 into dev/0.14.1 Jul 30, 2019
@drewbanin drewbanin deleted the fix/bq-repeated-record-execute branch July 30, 2019 20:27
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.

2 participants