eladkal commented on a change in pull request #22391:
URL: https://github.com/apache/airflow/pull/22391#discussion_r831441457
##########
File path: airflow/providers/amazon/aws/operators/redshift_sql.py
##########
@@ -74,5 +75,12 @@ def get_hook(self) -> RedshiftSQLHook:
def execute(self, context: 'Context') -> None:
"""Execute a statement against Amazon Redshift"""
self.log.info(f"Executing statement: {self.sql}")
+ sql_stmts = sqlparse.split(self.sql)
hook = self.get_hook()
- hook.run(self.sql, autocommit=self.autocommit,
parameters=self.parameters)
+
+ with hook.get_conn() as con:
+ con.autocommit = self.autocommit
+ with con.cursor() as cursor:
+ for stmt in sql_stmts:
+ cursor.execute(stmt)
+ if self.autocommit is False: con.commit()
Review comment:
Not quite...
The function `run` comes from:
https://github.com/apache/airflow/blob/900bad1c67654252196bb095a2a150a23ae5fc9a/airflow/hooks/dbapi.py#L163
your suggestion to not use it and set `run_sql` means that now the hook has
two ways to execute sql. This is not ideal.
You should make it work with `run`. You can override the `run` by having a
new implementation in `RedshiftSQLHook` (Just like we do it for Snowflake in
the PR I shared)
To be specific:
The operator calling `hook.run()` should stay exactly as it is. Let me also
point that by removing `parameters=self.parameters` in the operator you are
forcing breaking change which is something that we avoid unless absolutely
necessary (and I don't think this is a case where it's necessary correct me if
i'm wrong)
So the hook needs to handle single query or multiple.
--
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]
