Taragolis commented on code in PR #28744: URL: https://github.com/apache/airflow/pull/28744#discussion_r1063988859
########## airflow/providers/exasol/operators/exasol.py: ########## @@ -17,12 +17,15 @@ # under the License. from __future__ import annotations -import warnings from typing import Sequence from airflow.providers.common.sql.operators.sql import SQLExecuteQueryOperator +def exasol_fetch_all_handler(cursor) -> list[tuple] | None: + return cursor.fetchall() Review Comment: > do not think this is a problem - we only really should care about the non-compliance if we are depending on it in our code - otherwise, it's up to the user to know that Presto is not fully db-api-2 compliant. In case of exasol, the root cause was that it does not set the ".description" property which we used to determine if we should fetch results or not, and I believe we have not used paramstyle in our "common.sql" code. Yep most agree with this with a small concern then we should use all attributes/methods of DB-API 2 first rather than implement our solution because there is always a risk that we reinvent sqlalchemy If we potentially can use `paramstyle` it would allow us not reinvent the wheel. Right now we by default use that by DB use `format` (ANSI C printf format codes, e.g. `...WHERE name=%s`) and if DB use different style than we overwrite this argument for specific hook https://github.com/apache/airflow/blob/3eee33ac8cb74cfbb08bce9090e9c601cf98da44/airflow/providers/common/sql/hooks/sql.py#L126-L127 Unfortunetly for some reason not all backends implement this attribute such as `presto-python-client` as well as `pyexasol` which also for some reason not implements params for [execute](https://github.com/exasol/pyexasol/blob/master/pyexasol/db2/__init__.py#L37-L38). Reuse mandatory DB-API 2 parameters and methods would make our life easier in case of if Hook support different backends, for example MySqlHook could use `mysql-connector-python` and `mysqlclient` fortunately both of them use same param style. In some time in the future we would in position when we need support both `psycopg2` and `psycopg` (v3) for PostgresHook, but we also lucky with this backends, both of them support `pyformat` (specified in DB-API adapter) and `format` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
