Add hook_params to SqlSensor.
#18431
Conversation
|
Hi, @SamWheating. |
airflow/models/connection.py
Outdated
There was a problem hiding this comment.
It is discouraged to pass a mutable object as a default. See this SO https://stackoverflow.com/q/1132941/621058 . Instead you can null it by default and if it's null assign it later
def get_hook(self, hook_params: Dict = None):
if hook_params is None:
hook_params = {}
# ...There was a problem hiding this comment.
Yeah, I've also had to deal with the problem described in SO.
But in this case, Dict is just a container and hasn't been updated in the method.
Could you please write an example in the context of SqlSensor when we can receive the behavior similar to SO?
Because now, in my opinion, it looks like code sophistication.
There was a problem hiding this comment.
I understand your point @kazanzhy but I agree we should just stick with the practice of using None as the default for Optional[Dict] that is, as far as I know, uniformly adhered to in this repo.
If you leave it, people will always give it a second look when they see it. And pycharm complains about it. And using None for an optional arg is also a common pattern that I think is good aside from the mutability issue, just sends the message, you don't need to pass this, and we don't forward anything in this case.
|
Hello there. Been on holiday for some time, thats why #17592 was stalled. I see you get good progress here, including adding the missing DbHooks. Tell me If I can contribute somehow, just don't want to duplicate efforts |
|
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. |
github-actions
bot
added
the
stale
kaxil
added
pinned
dstandish
left a comment
dstandish
left a comment
There was a problem hiding this comment.
I think this needs to be split into 2 prs. This one should only add hook_params. Changing the conn types should be done separately.
airflow/models/connection.py
Outdated
There was a problem hiding this comment.
I understand your point @kazanzhy but I agree we should just stick with the practice of using None as the default for Optional[Dict] that is, as far as I know, uniformly adhered to in this repo.
If you leave it, people will always give it a second look when they see it. And pycharm complains about it. And using None for an optional arg is also a common pattern that I think is good aside from the mutability issue, just sends the message, you don't need to pass this, and we don't forward anything in this case.
airflow/sensors/sql.py
Outdated
kazanzhy
requested review from
bbovenzi,
dimberman,
ephraimbuddy,
jedcunningham,
jhtimmins,
mik-laj,
potiuk,
ryanahamilton,
turbaszek,
uranusjr and
vikramkoka
as code owners
kazanzhy
force-pushed
the
sqlsensor_hook_params
branch
from
ec1586c to
bb3ac65
Compare
kazanzhy
changed the title
hook_kwargs and fix allowed_conn_types for SqlSensor.
kazanzhy
changed the title
hook_kwargs and fix allowed_conn_types for SqlSensor.hook_params and fix allowed_conn_types for SqlSensor.
kazanzhy
force-pushed
the
sqlsensor_hook_params
branch
from
ca1e4f9 to
14562be
Compare
|
I updated the description. Also changed name of the parameter to |
kazanzhy
changed the title
hook_params and fix allowed_conn_types for SqlSensor.hook_params to SqlSensor.
kazanzhy
force-pushed
the
sqlsensor_hook_params
branch
from
459a652 to
f7abbfc
Compare
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
kazanzhy
force-pushed
the
sqlsensor_hook_params
branch
from
f7abbfc to
a2fa615
Compare
kazanzhy
force-pushed
the
sqlsensor_hook_params
branch
from
a2fa615 to
648254d
Compare
|
Awesome work, congrats on your first merged pull request! |
10 participants
This PR is based on: #17592 and closes: #13750
SqlSensor relies on underlying hooks that are automatically injected (DI) depending on the connection type provided to the SqlSensor. However, from time to time (but mainly in BigqueryHook) it is needed to pre-configure the "to-be-injected" hook because some default params do not fit the current config.
For example, if using SqlSensor with Bigquery it by default configures the SQL dialect to "legacy SQL", and there is no way to change this through configuration. Instead, the SqlSensor has to be extended and some functions are overwritten.