-
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
Fix placeholders in TrinoHook
, PrestoHook
, SqliteHook
#25939
Fix placeholders in TrinoHook
, PrestoHook
, SqliteHook
#25939
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
…n with db-specific values
…use of the new placeholder class attribute in DbApiHook
TrinoHook
, PrestoHook
, SqliteHook
This is cool. I believe you will also have to bump common-sql provider to 1.2.0 for it in order to make CI pass. |
Any idea on why that one CI check has failed? |
Random GA glitch. retarted it |
@@ -109,6 +109,8 @@ class DbApiHook(BaseForDbApiHook): | |||
connector = None # type: Optional[ConnectorProtocol] | |||
# Override with db-specific query to check connection | |||
_test_connection_sql = "select 1" | |||
# Override with the db-specific value used for placeholders | |||
placeholder: str = "%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it reference to mandatory DB-API 2 paramstyle property?
Python 3.9.10 (main, Feb 25 2022, 16:54:01)
Type 'copyright', 'credits' or 'license' for more information
IPython 8.4.0 -- An enhanced Interactive Python. Type '?' for help.
PyDev console: using IPython 8.4.0
Python 3.9.10 (main, Feb 25 2022, 16:54:01)
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
import sqlite3, trino, prestodb, psycopg2
sqlite3.paramstyle
Out[3]: 'qmark'
trino.dbapi.paramstyle
Out[4]: 'qmark'
psycopg2.paramstyle
Out[5]: 'pyformat'
prestodb.dbapi.paramstyle
Traceback (most recent call last):
File "/Users/taragolis/.pyenv/versions/airflow-dev-env-39/lib/python3.9/site-packages/IPython/core/interactiveshell.py", line 3398, in run_code
exec(code_obj, self.user_global_ns, self.user_ns)
File "<ipython-input-6-cb48c8aec499>", line 1, in <cell line: 1>
prestodb.dbapi.paramstyle
AttributeError: module 'prestodb.dbapi' has no attribute 'paramstyle'
Awesome work, congrats on your first merged pull request! |
@@ -22,6 +22,7 @@ description: | | |||
`Common SQL Provider <https://en.wikipedia.org/wiki/SQL>`__ | |||
|
|||
versions: | |||
- 1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@potiuk This feels like it should be a 1.1.1 -- i.e. a bug fix release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's fine. There is new feature (placeholder) added to common.sql . The bugfix bumps should be in trino, presto, sqlite, but common-sql has a new feature added and those bugfix versions of trino/presto/sqlite should depend on this new feature. All looks fine here.
closes: #25937
Added a class attribute to
DbApiHook
with the placeholder value to use in insert queries.Fixed
TrinoHook
andPrestoHook
placeholder values (from the default "%s" to "?")Updated
SqliteHook
to use the new placeholder parameter.