-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Remove obsolete and deprecated bigquery native write. #23557 #23558
Changes from 2 commits
ff29677
c318966
b9bd7b7
1e838db
482229d
989b682
31d4998
7c81c44
0c48d23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1410,7 +1410,7 @@ def __next__(self): | |
|
||
|
||
@deprecated(since='2.11.0', current="WriteToBigQuery") | ||
class BigQuerySink(*args, **kwargs): | ||
def BigQuerySink(*args, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why change this from class to def? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This way one can still do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it makes sense, add a test to make sure this fallback works. Since any code calling the fallback will expect the same interface as the deprecated BigQuerySink right? Is the interface exactly the same, or could users get runtime errors in unexpected places? If there are unexpected runtime errors consider fully sunsetting instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is intended to be a drop-in replacement, with the old variant deprecated over two years ago. |
||
"""A deprecated alias for WriteToBigQuery.""" | ||
warnings.warn( | ||
"Native sinks no longer implemented; " | ||
|
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.
Maybe comment that old code using this variant will redirect to the new variant with a warning. This isn't a breaking change right?
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.
The same line added in #23557 introduces a conflict. Could just change this line to
during rebase.
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.
Yeah, I merged these lines in the changelog.