Skip to content

Conversation

@lwyszomi
Copy link
Contributor

@lwyszomi lwyszomi commented Mar 1, 2022

Update package to give users possibility to install sqlalchemy-spanner.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@lwyszomi
Copy link
Contributor Author

lwyszomi commented Mar 1, 2022

@potiuk Could you assign someone to review?

@mik-laj
Copy link
Member

mik-laj commented Mar 1, 2022

To add support for sqlalchemy-spanner, we should also add get_uri method.

def get_uri(self) -> str:
"""Override DbApiHook get_uri method for get_sqlalchemy_engine()"""
conn_params = self._get_conn_params()
return self._conn_params_to_sqlalchemy_uri(conn_params)

@potiuk
Copy link
Member

potiuk commented Mar 6, 2022

Rebased it to latest main - let's see if it works.

@lwyszomi
Copy link
Contributor Author

lwyszomi commented Mar 7, 2022

@potiuk @mik-laj I was offline most of the week, Thanks for feedback I will update my PR in this week.

@lwyszomi
Copy link
Contributor Author

@mik-laj When we add get_uri method we should also extend this hook and add inheritance from DbApiHook?

@mik-laj
Copy link
Member

mik-laj commented Mar 14, 2022

@lwyszomi , Yes. Thanks to this, we will be able to use it in other places where DBApiHook class is expected. Users will also not need to study this class specifically, as they will be able to rely on a unified interface.

@lwyszomi
Copy link
Contributor Author

@mik-laj I have one more think in my mind, if we extend this hook maybe by default we should add sqlalchemy-spanner to the google provider packages, WDYT?

@mik-laj
Copy link
Member

mik-laj commented Mar 15, 2022

@lwyszomi Yes. We should do it. Thanks to this, we will avoid dependency conflicts, because problems will be detected on the CI.

airflow/setup.py

Lines 513 to 516 in 774ca08

snowflake = [
'snowflake-connector-python>=2.4.1',
'snowflake-sqlalchemy>=1.1.0',
]

@potiuk
Copy link
Member

potiuk commented Mar 16, 2022

Yep. Good idea.

@lwyszomi
Copy link
Contributor Author

@potiuk @mik-laj This change for now is blocked because we have a issue with the dependencies. The Airflow supporting sqlalchemy<1.4.10,>=1.4 but sqlalchemy-spanner supporting sqlalchemy<=1.3.23,>=1.1.13. Conflict for now cannot be resolved, we need to wait when sqlalchemy-spanner will support sqlalchemy<1.4.10,>=1.4.

@potiuk
Copy link
Member

potiuk commented Mar 16, 2022

@potiuk @mik-laj This change for now is blocked because we have a issue with the dependencies. The Airflow supporting sqlalchemy<1.4.10,>=1.4 but sqlalchemy-spanner supporting sqlalchemy<=1.3.23,>=1.1.13. Conflict for now cannot be resolved, we need to wait when sqlalchemy-spanner will support sqlalchemy<1.4.10,>=1.4.

Too bad. I opened issue for that: googleapis/python-spanner-sqlalchemy#204

@lwyszomi
Copy link
Contributor Author

@potiuk thanks

@potiuk
Copy link
Member

potiuk commented Mar 16, 2022

Apparently, support (and test case suite) was added in January but the setup.py has not been updated :)

googleapis/python-spanner-sqlalchemy#204 (comment)

🤷

@github-actions
Copy link

github-actions bot commented May 1, 2022

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 github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 1, 2022
@github-actions github-actions bot closed this May 7, 2022
@potiuk
Copy link
Member

potiuk commented Jun 6, 2022

@lwyszomi - seems like Google Spanner now suports sqlalchemy 1.4 googleapis/python-spanner-sqlalchemy#204 - feel free to provide support for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants