Skip to content

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Aug 12, 2017

Cherry-picked from PR #3721

@tseaver tseaver added the api: bigquery Issues related to the BigQuery API. label Aug 12, 2017
@tseaver tseaver requested review from dhermes and tswast August 12, 2017 19:34
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 12, 2017
@tseaver tseaver mentioned this pull request Aug 12, 2017
18 tasks
@tseaver
Copy link
Contributor Author

tseaver commented Aug 12, 2017

@tswast I noted your question about creating a to-be-written QueryPlanEntry class: here, because the values are "dumb data" (the class would have no methods except those to marshall instances from the JSON resource), I think it is better to just leave them as dicts.

This is different for referenced_tables and unused_query_parameters, where we already have domain objects which correspond to those resources.

'computeRatioMax': 1.09861,
'writeRatioAvg': 3.32193,
'writeRatioMax': 2.30258,
'recordsRead': 100,

This comment was marked as spam.

'writeRatioAvg': 3.32193,
'writeRatioMax': 2.30258,
'recordsRead': 100,
'recordsWritten': 1,

This comment was marked as spam.

@tswast
Copy link
Contributor

tswast commented Aug 14, 2017

Note: I still think a class would be useful for string to int conversion, but it was only a P2 requirement from BQ team, so I won't block the PR.

return job

@property
def query_plan(self):

This comment was marked as spam.

@tseaver
Copy link
Contributor Author

tseaver commented Sep 7, 2017

@tswast db24010 should address your requested changes.

Note: that commit adds 350 lines of code for very little tangible benefit: I can't imagine anybody writing code which depends on the fact that records_read / records_written are integers.

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.

Thanks!

The new class will definitely be a usability improvement over using dictionaries. I agree that usage will likely be low. That's why it was a P2 item in the requirements doc from the BQ eng team.

@tseaver tseaver merged commit 019b99f into googleapis:bigquery-b2 Sep 7, 2017
@tseaver tseaver deleted the bigquery-b2-queryjob-query_plan branch September 7, 2017 19:05
tseaver added a commit that referenced this pull request Sep 11, 2017
tswast pushed a commit that referenced this pull request Sep 25, 2017
tswast pushed a commit that referenced this pull request Oct 12, 2017
tswast pushed a commit that referenced this pull request Oct 16, 2017
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. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants