Snowflake hook doesn't parameterize SQL passed as a string type, causing SnowflakeOperator to fail

[grassten]

Apache Airflow version: 2.1.0

What happened: The new Snowflake hook run method is not taking parameters into account when the SQL is passed as a string; it's using Snowflake connector's execute_string method, which does not support parameterization. So the only way to parameterize your query from a SnowflakeOperator is to put the SQL into a list.

airflow/airflow/providers/snowflake/hooks/snowflake.py

Lines 272 to 279 in 304e174

	if isinstance(sql, str):
	cursors = conn.execute_string(sql, return_cursors=True)
	for cur in cursors:
	self.query_ids.append(cur.sfqid)
	self.log.info("Rows affected: %s", cur.rowcount)
	self.log.info("Snowflake query id: %s", cur.sfqid)
	cur.close()

https://docs.snowflake.com/en/user-guide/python-connector-api.html#execute_string

How to reproduce it: Pass a sql string and parameters to SnowflakeOperator; the query will not be parameterized, and will fail as a SQL syntax error on the parameterization characters, e.g. %(param)s.

Anything else we need to know: Quick workaround is to put your sql string into a list, these are still being parameterized correctly.

[boring-cyborg]

Thanks for opening your first issue here! Be sure to follow the issue template!

[potiuk]

Would you be willing to submit a fix @grassten ? Happy to review and merge it,

[grassten]

Would you be willing to submit a fix @grassten ? Happy to review and merge it,

Sure! I'll work on getting a PR in today or tomorrow morning.

[grassten]

@potiuk I'm wondering if we should split the passed sql param and then parameterize the resulting queries, or stop splitting the string altogether (splitting was implemented recently in #15533). To split the string I think we could use from snowflake.connector.util_text import split_statements; that's what's used by execute_string in the Snowflake connector. (I have that in a PR in my fork here)

That being said, I don't think parameterizing a list of queries (after splitting) with a single set of params is valid. I think we should put the passed sql into a single item list & pass it through the existing code; this seems more in line w/ what you would expect to happen. cc @mobuchowski @eladkal I see you guys on that PR, what do you think?

Edit: I've submitted the PR to use split_statements; I think this implementation makes strings w/ multiple statements behave similarly to lists of sql in its parameter handling.

[potiuk]

@mobuchowski - WDYT?