-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add UDF support for BigQuery #1881
Conversation
@ukarlsson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blacker, @mikekap and @mbruggmann to be potential reviewers. |
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.
Looks good. I have some basic questions and inline comments though: Can you add a test case? Have this been used in production?
@@ -594,7 +600,8 @@ def run(self): | |||
'allowLargeResults': True, | |||
'createDisposition': self.create_disposition, | |||
'writeDisposition': self.write_disposition, | |||
'flattenResults': self.flatten_results | |||
'flattenResults': self.flatten_results, | |||
'userDefinedFunctionResources': [{"resourceUri": v} for v in self.udf_resource_uris] |
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.
Add a trailing comma please
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.
Fixed.
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.
what happened to the trailing comma you added??
@@ -567,6 +567,12 @@ def query_mode(self): | |||
"""The query mode. See :py:class:`QueryMode`.""" | |||
return QueryMode.INTERACTIVE | |||
|
|||
@property | |||
def udf_resource_uris(self): | |||
"""List of code resource to load from a Google Cloud Storage URI (gs://bucket/path). |
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.
s/List/Iterable?
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.
Fixed.
I have added tests. Yes, we use this in production |
0f771ee
to
7d3d69a
Compare
@Tarrasch OK thanks have fixed the whitespacing and rebased :) |
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.
Did you mix up git branches or something? It seems like a previous amendment got lost. So I "red" this PR again so you can confirm that everything is OK before we merge.
Other than that this is all-merge for me. :)
@@ -594,7 +600,8 @@ def run(self): | |||
'allowLargeResults': True, | |||
'createDisposition': self.create_disposition, | |||
'writeDisposition': self.write_disposition, | |||
'flattenResults': self.flatten_results | |||
'flattenResults': self.flatten_results, | |||
'userDefinedFunctionResources': [{"resourceUri": v} for v in self.udf_resource_uris] |
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.
what happened to the trailing comma you added??
I merge as this have had many minor iterations already. But I see that @ukarlsson actually never filled out the template in the PR. While it's indeed optional, I think you could jot down some words about why your company needed this patch. What can be done now that you couldn't do before? Thanks for the patch! :) |
No description provided.