-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
BigQuery: Fix Add maximum_bytes_billed option to magics #8179
BigQuery: Fix Add maximum_bytes_billed option to magics #8179
Conversation
Raises: | ||
ValueError: If the parameters are invalid. | ||
""" | ||
return self._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 be return 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 a self._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.
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 a default_query_job_config
property is the better implementation.
I've added commit 30f8326 that implements this.
def maximum_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 need self._maximum_bytes_billed
? Can't we just use 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.
Why is job_config
even in this function?
One way of implementing this feature would be to create a self._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 | ||
with run_query_patch as run_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 the maximum_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 the maximum_bytes_billed
is correct?
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.
def maximum_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 is job_config
even in this function?
One way of implementing this feature would be to create a self._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 | ||
|
||
except Exception: |
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 let int()
throw.
try: | ||
value = int(args.maximum_bytes_billed) | ||
job_config.maximum_bytes_billed = value | ||
except Exception: |
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 catch Exception
. 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. | ||
""" | ||
return self._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 a self._default_job_config
object, but I think that can wait until we have more than one default property to set.
e140b64
to
30f8326
Compare
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.
Thank you @HemangChothani for the contribution. This feature will appear in the next release of |
Fixes #7678