[GitHub] [airflow] eladkal commented on a diff in pull request #23971: Unify DbApiHook.run() method with the methods which override it

[GitBox <gi...@apache.org>]

eladkal commented on code in PR #23971:
URL: https://github.com/apache/airflow/pull/23971#discussion_r920486414


##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -213,14 +243,13 @@ def run(self, sql, autocommit=False, parameters=None, handler=None):
             before executing the query.
         :param parameters: The parameters to render the SQL query with.
         :param handler: The result handler which is called with the result of each statement.
-        :return: query results if handler was provided.
+        :return: return only result of the LAST SQL expression if handler was provided.
         """
-        scalar = isinstance(sql, str)
-        if scalar:
-            sql = [sql]
+        if isinstance(sql, str):
+            sql = self.maybe_split_sql_string(sql)

Review Comment:
   I think this is a big behavior change (?)
   
   If we take `PostgersOperator` for example.
   When you pass `SELECT 1; SELECT2;` The two statements will be executed in a single query.
   Now, this will be changed to `["SELECT 1", "SELECT2;"]` which will be executed in two separate queries.
   This is a significant change!
   
   There are DBs where this is the only way (Presto, Snowflake, Trino...) so there we don't have this issue but in Postgres, MySQL `sql="SELECT 1; SELECT2;"` ! = `sql=["SELECT 1", "SELECT2;"]` these are two distinct intentions from the user (while can and probably will be resulted in the same end state, it might not be the case if the there is a rollback or some other issue in the middle of execution_
   
   I **think** that what we want here is that the run function to also accept a `split_statments=False` parameter and we will set it only in specific providers (Presto, Trino, Snowflake, etc..) 



##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -201,7 +213,25 @@ def get_first(self, sql, parameters=None):
                     cur.execute(sql)
                 return cur.fetchone()
 
-    def run(self, sql, autocommit=False, parameters=None, handler=None):
+    @staticmethod
+    def maybe_split_sql_string(sql: str) -> List[str]:
+        """
+        Splits strings consisting of multiple SQL expressions into an

Review Comment:
   The docstring is incomplete



##########
airflow/providers/common/sql/hooks/sql.py:
##########
@@ -201,7 +213,25 @@ def get_first(self, sql, parameters=None):
                     cur.execute(sql)
                 return cur.fetchone()
 
-    def run(self, sql, autocommit=False, parameters=None, handler=None):
+    @staticmethod
+    def maybe_split_sql_string(sql: str) -> List[str]:

Review Comment:
   I think we can just call this function `split_sql_string`?



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org