Skip to content
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 sqlalchemy_url property to DbApiHook class #38871

Merged
merged 23 commits into from
Apr 14, 2024

Conversation

rawwar
Copy link
Contributor

@rawwar rawwar commented Apr 9, 2024

related: #38195

This PR intends to add a method get_sqlalchemy_url, which is dependent on a property named drivername that must be defined in any provider subclass that wants to generate a SqlAlchemy URI.

This PR intends to add a property sqlalchemy_url which must be defined in any provider subclass that wants to generate a SqlAlchemy URI.

Related discussions:

Signed-off-by: kalyanr <kalyan.ben10@live.com>
@rawwar
Copy link
Contributor Author

rawwar commented Apr 9, 2024

As of now, there are tests failing because 'drivername' property is not implemented in the providers. I can implement them in the subclasses in this PR itself if this approach is ok to go ahead.

@Taragolis
Copy link
Contributor

I just keep part of my comment here

After start implements into the other hooks with potential usage in particular this hook. And after it implements in many (more that 50% I guess) community supported hooks we might start use it internally in DBApiHook, e.g. obtain SA Connections, pandas dataframes and others, with backward compatible logic to the previous implementation for prevent case if hook do not implements this one and for some unknown reason it work (or not) previously. So we could reduce number of potential mistakes which spread across other hooks.

24 Hooks (approx) have no idea how to use it right now that is expected that it failed. The idea is create an interface, without any implementation, and make related hooks decide whether or not implement it, if it not implemented in most providers, there is no reason to create any of implementation into the DBApiHook, it harmful and produce more problem rather than solves

airflow/providers/common/sql/hooks/sql.py Outdated Show resolved Hide resolved
airflow/providers/common/sql/hooks/sql.py Outdated Show resolved Hide resolved
@rawwar
Copy link
Contributor Author

rawwar commented Apr 10, 2024

I just keep part of my comment here

After start implements into the other hooks with potential usage in particular this hook. And after it implements in many (more that 50% I guess) community supported hooks we might start use it internally in DBApiHook, e.g. obtain SA Connections, pandas dataframes and others, with backward compatible logic to the previous implementation for prevent case if hook do not implements this one and for some unknown reason it work (or not) previously. So we could reduce number of potential mistakes which spread across other hooks.

24 Hooks (approx) have no idea how to use it right now that is expected that it failed. The idea is create an interface, without any implementation, and make related hooks decide whether or not implement it, if it not implemented in most providers, there is no reason to create any of implementation into the DBApiHook, it harmful and produce more problem rather than solves

Thank you. That makes sense.

@rawwar rawwar changed the title Add get_sqlalchemy_url method for SQLAlchemy URI generation Add sqlalchemy_url property in DbApiHook class Apr 10, 2024
@rawwar rawwar marked this pull request as ready for review April 10, 2024 09:40
@rawwar rawwar requested a review from eladkal as a code owner April 10, 2024 09:40
@rawwar rawwar changed the title Add sqlalchemy_url property in DbApiHook class Add sqlalchemy_url property to DbApiHook class Apr 10, 2024
@rawwar rawwar requested a review from Taragolis April 10, 2024 12:11
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
@rawwar rawwar requested a review from Taragolis April 12, 2024 12:40
@rawwar
Copy link
Contributor Author

rawwar commented Apr 14, 2024

@Taragolis , can this be merged? I can continue working on PR's to update other providers

@Taragolis Taragolis merged commit 08f4b92 into apache:main Apr 14, 2024
41 checks passed
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
* Add sqlalchemy_url property to DbApiHook

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* update get_sqlalchemy_url docstring

* update drivername property docstring

* fix spelling

* add sqlalchemy_url property to DbApiHook

* fix typo

* precommit failure fix

Signed-off-by: kalyanr <kalyan.ben10@live.com>

* Update sql.py

Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>

* fix error message

---------

Signed-off-by: kalyanr <kalyan.ben10@live.com>
Co-authored-by: Andrey Anshin <Andrey.Anshin@taragol.is>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants