Skip to content
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

SnowflakeSqlApiOperator not resolving parameters in SQL #42033

Closed
chris-okorodudu opened this issue Sep 5, 2024 · 3 comments · Fixed by #42719
Closed

SnowflakeSqlApiOperator not resolving parameters in SQL #42033

chris-okorodudu opened this issue Sep 5, 2024 · 3 comments · Fixed by #42719
Labels
area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet provider:snowflake Issues related to Snowflake provider

Comments

@chris-okorodudu
Copy link

Apache Airflow version

2.9.3

If "Other Airflow 2 version" selected, which one?

No response

What happened?

The SnowflakeSqlApiOperator does not resolve parameters in SQL despite accepting this param:

:param parameters: (optional) the parameters to render the SQL query with.

This is due to the fact that it executes by initializing a SnowflakeSqlApiHook and then executing the queries without ever passing the parameters:

        self._hook = SnowflakeSqlApiHook(
            snowflake_conn_id=self.snowflake_conn_id,
            token_life_time=self.token_life_time,
            token_renewal_delta=self.token_renewal_delta,
            deferrable=self.deferrable,
            **self.hook_params,
        )
        self.query_ids = self._hook.execute_query(
            self.sql,  # type: ignore[arg-type]
            statement_count=self.statement_count,
            bindings=self.bindings,
        )

This means that parameters passed in and then referenced how they would be in other Snowflake operators - %(param)s - will not be resolved and cause the execution to fail.

What you think should happen instead?

The parameters should be resolved either before the sql is passed to the SnowflakeSqlApiHook, or as part of the SnowflakeSqlApiHook.

How to reproduce

To reproduce, try passing any parameter and referencing it in your SQL via this syntax %(param)s

Operating System

all

Versions of Apache Airflow Providers

Tested with multiple versions, most recently

apache-airflow-providers-snowflake==5.6.1

### Deployment

Official Apache Airflow Helm Chart

### Deployment details

Tested this both with the official helm chart locally and with MWAA. Issue occurred in both.

### Anything else?

I was able to get it working by adding these lines 
    quoted_params = {k: f"'{v}'" for k, v in self.parameters.items()}
    rendered_sql = self.sql % quoted_params
    self.sql = rendered_sql
before the call to 
    self.query_ids = self._hook.execute_query(
        self.sql,  # type: ignore[arg-type]
        statement_count=self.statement_count,
        bindings=self.bindings,
    )

However, it may make more sense for the parameters to be passed to execute_query and resolved there, which would require an update to the SnowflakeSqlApiHook instead. Let me know if there's some other way to pass the params that I'm missing.

### Are you willing to submit PR?

- [X] Yes I am willing to submit a PR!

### Code of Conduct

- [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
@chris-okorodudu chris-okorodudu added area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Sep 5, 2024
Copy link

boring-cyborg bot commented Sep 5, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@dosubot dosubot bot added the provider:snowflake Issues related to Snowflake provider label Sep 5, 2024
harjeevanmaan added a commit to harjeevanmaan/airflow that referenced this issue Oct 3, 2024
…ement SQL requests.

- added warning for multi-statement query bindings in Snowflake hook
test: added unit test for multi-statement bindings warning

Related: apache#42033
@harjeevanmaan
Copy link
Contributor

harjeevanmaan commented Oct 3, 2024

@chris-okorodudu You can add bindings as a keyword argument within self._hook.execute_query.

Here is an example:

"bindings": {
  "1": {
    "type": "FIXED",
    "value": "123"
  }
}

or more details on the correct format, please refer to the following article: sql-api-bind-variables
Please be mindful that Snowflake does not currently support bindings in multi-statement SQL requests.

potiuk pushed a commit that referenced this issue Oct 4, 2024
…ement SQL requests. (#42719)

- added warning for multi-statement query bindings in Snowflake hook
test: added unit test for multi-statement bindings warning

Related: #42033
@potiuk
Copy link
Member

potiuk commented Oct 4, 2024

While no entirely fixed - seems that this is on the snowflake side and #42719 at least provides an explanation.

@potiuk potiuk closed this as completed Oct 4, 2024
joaopamaral pushed a commit to joaopamaral/airflow that referenced this issue Oct 21, 2024
…ement SQL requests. (apache#42719)

- added warning for multi-statement query bindings in Snowflake hook
test: added unit test for multi-statement bindings warning

Related: apache#42033
ellisms pushed a commit to ellisms/airflow that referenced this issue Nov 13, 2024
…ement SQL requests. (apache#42719)

- added warning for multi-statement query bindings in Snowflake hook
test: added unit test for multi-statement bindings warning

Related: apache#42033
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet provider:snowflake Issues related to Snowflake provider
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants