-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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 cursor methods calls from GCSToBigQueryOperator #20119
Conversation
c510522
to
02e9f8f
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
2889bb4
to
c8a5705
Compare
c8a5705
to
3bc84f4
Compare
Warnings in BQ hook was added by this commit |
WDYT @turbaszek :) ? |
e7a31ae
to
e9ff9db
Compare
e9ff9db
to
e9f0639
Compare
Rebased it to make sure it is still ok. |
e9f0639
to
9f83496
Compare
Those dicts should be created by users. If we create them then we tend to miss some input arguments. Also in my opinion one input which is well described by Google docs is better than 20 arguments with copy pasted descriptions. |
@turbaszek |
Correct, and I believe we have separated warning in operators. Users can use both hook and operators and in both cases should be informed. |
4201e45
to
a4f0923
Compare
So I returned warnings back to the BQ hook and suppressed them from the operator. Also, I think there are left no usages of |
a4f0923
to
151ba75
Compare
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
Very good work with the #24363. As I wrote earlier in this discussion it should be the place with all this routine dict creation. My previous idea was to keep it in hook, but probably we could think about the handler. |
I'd love to work on this. But require more context on this issue. From my understanding, both |
here's the part of my DAG
This pattern allows to create a dict for the |
closes: #10288
I've tried to remove warnings during
GCSToBigQueryOperator
as described in issue #10288.As a result, I copied almost all code from
bq_hook.create_external_table
andbq_hook.run_load
which made the operator'sexecute
method very sophisticated.That's why I just switched from
cursor
method calls toBigQueryHook
methods. In my opinion, these methods should not be deprecated.We need someplace for complex dict creation, like
configuration
forbq_hook.insert_job()
ortable_recource
forbq_hook.create_empty_table()
. In my opinion, better to keep this code in BigQueryHook and don't move to Operator.