- Notifications
You must be signed in to change notification settings - Fork1.6k
BigQuery: Fix Add maximum_bytes_billed option to magics#8179
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
BigQuery: Fix Add maximum_bytes_billed option to magics#8179
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| Raises: | ||
| ValueError: If the parameters are invalid. | ||
| """ | ||
| returnself._maximum_bytes_billed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't this bereturn job_config.maximum_bytes_billed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this part is OK. A potential alternative implementation is to have aself._default_job_config object, but I think that can wait until we have more than one default property to set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should just have a default job config that users can set properties on. That way, users can set properties on the default config that we don't have magics arguments for. No need to make properties for each of the job config properties. That approach doesn't scale well, because it would be too much repetitive code to add each job config property separately.
HemangChothaniMay 29, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@tswast I have created a property for the class Context because i found Set job_config.maximum_bytes_billed = google.cloud.bigquery.magics.context.maximum_bytes_billed line in the description of feature request,So can i remove that property from Context class which is in magic.py file, What and where to set a defalut value or a value of google.cloud.bigquery.magics.context.maximum_bytes_billed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for the "flip flopping"@HemangChothani. Alix convinced me offline that adefault_query_job_config property is the better implementation.
I've added commit30f8326 that implements this.
| defmaximum_bytes_billed(self,value): | ||
| try: | ||
| value=int(value) | ||
| self._maximum_bytes_billed=value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we needself._maximum_bytes_billed? Can't we just usejob_config.maximum_bytes_billed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why isjob_config even in this function?
One way of implementing this feature would be to create aself._default_job_config to hold the default job config (used when constructing the BigQuery client), but that doesn't appear to be what's going on here.
bigquery/tests/unit/test_magics.py Outdated
| query_job_mock.to_dataframe.return_value=result | ||
| withrun_query_patchasrun_query_mock,default_patch: | ||
| run_query_mock.return_value=query_job_mock | ||
| return_value=ip.run_cell_magic("bigquery","--maximum_bytes_billed=123456789",sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please check to make sure that themaximum_bytes_billed is used?
bigquery/tests/unit/test_magics.py Outdated
| return_value=ip.run_cell_magic("bigquery","--maximum_bytes_billed=None",sql) | ||
| bqstorage_mock.assert_not_called() | ||
| query_job_mock.to_dataframe.assert_called_once_with(bqstorage_client=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please check to make sure that themaximum_bytes_billed is correct?
tswast left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 To Solomon's requests for test changes that verify the value is actually set.
| defmaximum_bytes_billed(self,value): | ||
| try: | ||
| value=int(value) | ||
| self._maximum_bytes_billed=value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why isjob_config even in this function?
One way of implementing this feature would be to create aself._default_job_config to hold the default job config (used when constructing the BigQuery client), but that doesn't appear to be what's going on here.
| job_config=bigquery.job.QueryJobConfig() | ||
| job_config.maximum_bytes_billed=self._maximum_bytes_billed | ||
| exceptException: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This catch block is not needed. Just letint() throw.
| try: | ||
| value=int(args.maximum_bytes_billed) | ||
| job_config.maximum_bytes_billed=value | ||
| exceptException: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ditto. Don't catchException. We do need to think about having better error messages in our magics, but I don't think this is the way to do it.
| Raises: | ||
| ValueError: If the parameters are invalid. | ||
| """ | ||
| returnself._maximum_bytes_billed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this part is OK. A potential alternative implementation is to have aself._default_job_config object, but I think that can wait until we have more than one default property to set.
e140b64 to30f8326CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tswast commentedJun 3, 2019
Coverage failure is a weird one. It's not happening locally. That line is: Maybe it's getting confused by the generator expression? I can try converting that to a plain old for loop. |
Maybe that'll fix the coverage issues?
Addressed requested changes in latest commits.
tswast commentedJun 3, 2019
Thank you@HemangChothani for the contribution. This feature will appear in the next release of |
Uh oh!
There was an error while loading.Please reload this page.
Fixes#7678