Skip to content

Comments

Add Architecture Decision Record for common.sql introduction#36015

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-common-sql-adr
Dec 6, 2023
Merged

Add Architecture Decision Record for common.sql introduction#36015
potiuk merged 1 commit intoapache:mainfrom
potiuk:add-common-sql-adr

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 1, 2023

Fixes: #35874


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk requested review from ashb and eladkal as code owners December 1, 2023 18:07
@potiuk potiuk removed the request for review from ashb December 1, 2023 18:07
@potiuk potiuk removed the request for review from eladkal December 1, 2023 18:07
@potiuk potiuk requested a review from josh-fell December 1, 2023 18:07
@potiuk potiuk force-pushed the add-common-sql-adr branch from 9dfde8c to abae161 Compare December 1, 2023 18:10
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good generally, few nit picking comments on usage of DBApi vs DBAPI

Copy link
Collaborator

@aritra24 aritra24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, some nits

@potiuk potiuk force-pushed the add-common-sql-adr branch from 461a0a3 to 9a7a10f Compare December 5, 2023 09:39
@Taragolis
Copy link
Contributor

I have some questions, I don't have any suggestions yet, but after some discussion we might have the right answer and add to this document, or upcoming updates.

  1. Any idea how we could resolve situation that some databases have multiple different drivers. In the same Hook or in the different one? e.g postgres have 3 quite popular (sync implementation)
  • psycopg2 - More common driver nowadays, use libpq
  • psycopg - formally psycopg3 but all future versions v4, v5, and etc. would be released as part of this package
  • pg8000 - python implementation of postgres protocol, do not required/use libpq
  1. Some drivers also implements both interfaces DBAPIv2 compliant and "internal" which usual faster, and have bigger functionality, what our preferences here?

  2. Not all DB have sqlaclhemy support. However in the current implementation of DbApiHook.get_sqlalchemy_engine. Any thought how we could make it more clear? And maybe we should remove default implementation and raise NotImplemented instead?

  3. Broken get_uri mechanism in DBAPIHook. And right now it also not clarify what kind of URI it expected to get: driver or sqlalchemy? What we should do to improve it? For the reference and details: https://lists.apache.org/thread/8rhmz3qh30hvkondct4sfmgk4vd07mn5

  4. Async implementation of drivers, this is extending of question 1. Most of the drivers supports either sync or async but not both, e.g. psycopg have support both but in addition also quite popular asyncpg or aiopg. Should we have separate hook for async drivers or we try to extend DbApiHook and make it really big an complicated?

@potiuk
Copy link
Member Author

potiuk commented Dec 6, 2023

I have some questions, I don't have any suggestions yet, but after some discussion we might have the right answer and add to this document, or upcoming updates.

  1. Any idea how we could resolve situation that some databases have multiple different drivers. In the same Hook or in the different one? e.g postgres have 3 quite popular (sync implementation)
  • psycopg2 - More common driver nowadays, use libpq
  • psycopg - formally psycopg3 but all future versions v4, v5, and etc. would be released as part of this package
  • pg8000 - python implementation of postgres protocol, do not required/use libpq
  1. Some drivers also implements both interfaces DBAPIv2 compliant and "internal" which usual faster, and have bigger functionality, what our preferences here?
  2. Not all DB have sqlaclhemy support. However in the current implementation of DbApiHook.get_sqlalchemy_engine. Any thought how we could make it more clear? And maybe we should remove default implementation and raise NotImplemented instead?
  3. Broken get_uri mechanism in DBAPIHook. And right now it also not clarify what kind of URI it expected to get: driver or sqlalchemy? What we should do to improve it? For the reference and details: https://lists.apache.org/thread/8rhmz3qh30hvkondct4sfmgk4vd07mn5
  4. Async implementation of drivers, this is extending of question 1. Most of the drivers supports either sync or async but not both, e.g. psycopg have support both but in addition also quite popular asyncpg or aiopg. Should we have separate hook for async drivers or we try to extend DbApiHook and make it really big an complicated?

Yep. I think those are great questions and I propose to add and discuss separate ADR entry for each of those questions - this is precisely what ADRs are for - separate ADR for each such decision. I think what we did with comon.sql was just "basic" commonalisation" and we shoudl continue with all the points you raised. I will merge it now. And we can discuss each of thsoe separately and propose ADRs for them individually.

@potiuk potiuk merged commit 3bb5978 into apache:main Dec 6, 2023
@potiuk potiuk deleted the add-common-sql-adr branch December 6, 2023 20:36
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.

Document the purpose of having common.sql

6 participants