[GitHub] [airflow] hussein-awala commented on pull request #29599: fix do_xcom_push=False bug in SnowflakeOperator

["hussein-awala (via GitHub)" <gi...@apache.org>]

hussein-awala commented on PR #29599:
URL: https://github.com/apache/airflow/pull/29599#issuecomment-1435737748

   > @hussein-awala - the bug _is_ specifically in the SnowflakeProvider - the base class just does a no-op with `_process_output` and isn't affected, I was hesitant to add it there in case the fix interferes with other providers some how by skipping a `_process_output` that does do something with an empty result set thinking
   > 
   > ```
   > --- a/airflow/providers/common/sql/operators/sql.py
   > +++ b/airflow/providers/common/sql/operators/sql.py
   > @@ -265,6 +265,9 @@ class SQLExecuteQueryOperator(BaseSQLOperator):
   >              return_last=self.return_last,
   >              **extra_kwargs,
   >          )
   > +        # Handle do_xcom_push=False
   > +        if output is None or output == [None]:
   > +            return []
   >          if return_single_query_results(self.sql, self.return_last, self.split_statements):
   >              # For simplicity, we pass always list as input to _process_output, regardless if
   >              # single query results are going to be returned, and we return the first element
   > 
   > --- a/airflow/providers/snowflake/operators/snowflake.py
   > +++ b/airflow/providers/snowflake/operators/snowflake.py
   > @@ -108,9 +108,6 @@ class SnowflakeOperator(SQLExecuteQueryOperator):
   >          results: Optional[list[Any]],
   >          descriptions: list[Sequence[Sequence] | None]
   >      ) -> list[Any]:
   > -        # Handle do_xcom_push=False
   > -        if results is None or results == [None]:
   > -            return []
   >          validated_descriptions: list[Sequence[Sequence]] = []
   >          for idx, description in enumerate(descriptions):
   >              if not description:
   > ```
   I think adding a condition on the `SQLExecuteQueryOperator` without changing the snowflake operator is enough to solve the problem, since we don't need to process the result because we don't want to push it as a xcom:
   ```diff
   --- a/airflow/providers/common/sql/operators/sql.py
   +++ b/airflow/providers/common/sql/operators/sql.py
   @@ -265,6 +265,8 @@ class SQLExecuteQueryOperator(BaseSQLOperator):
                return_last=self.return_last,
                **extra_kwargs,
            )
   +        if not self.do_xcom_push:
   +            return None
            if return_single_query_results(self.sql, self.return_last, self.split_statements):
                # For simplicity, we pass always list as input to _process_output, regardless if
                # single query results are going to be returned, and we return the first element
   ```
   What do you think?


-- 
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