Expose snowflake query_id in snowflake hook and operator, support multiple statements in sql string #15533

mobuchowski · 2021-04-26T15:47:03Z

Expose snowflake query id to snowflake hook and snowflake operator.
This will allow us to collect data lineage for snowflake operator in Marquez project.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

boring-cyborg · 2021-04-26T15:47:07Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

potiuk

Interesting, and while I see why you want to copy the run method, it lacks support for multiple queries

potiuk · 2021-04-26T16:58:51Z

airflow/providers/snowflake/hooks/snowflake.py

There are two thigns I do not like here:

Copying the logic of the DBApi run() method. I do not think however we can do too much about it though, because we do not want to modify the DBApi internals and make the provider depends on future version of Airflow.. So not much we can do here.

We only store LAST query id even if we execute a sequence of queries. I think we should keep an array of those. rather than overwrite the query id with the last one

Related to point 1 - this problem (allow multiple queries) is not unique to Snowflake. This also applies to Presto and Trino so whatever solution that will be accepted here will most likely be copied to the other two providers as well.

Thanks for the feedback, I agree with both points and will adjust.
I did not find better way than copying without changing DBApi semantics, also, when looking for similar solution saw that that's what some connectors do already - like Exasol one.

We could potentially add some "callbacks" in the DBHook implementation, but this would mean Airflow 2.1+ compatibility. I am not sure it's worth it. Maybe we could figure out some ways of doing back-compatibility and implement it "properly" (like copying the method but use calbacks/original run with callbacks() when available? But I am not sure if it is worth it either.

That could possibly allow native OpenLineage integration in airflow, but that's separate topic 🙂

Maybe we could figure out some ways of doing back-compatibility and implement it "properly" (like copying the method but use calbacks/original run with callbacks() when available?

Yes. that makes sense - there's a lot of metadata that some databases expose in a non standard way. One example is BigQueryExecuteQueryOperator, which puts job id into xcoms.
For now, I don't think exposing query id in this particular way in this particular provider will make it in the future.

Open Lineage integration is a cool idea. @bolkedebruin might be interested :) . How about starting discussion about it at the devlist @mobuchowski ? Sounds like a very good idea.

We're discussing it internally and will come back with some ideas surely 🙂

Is it possible to save the query_ids as the inlets/outlets defined for the LineageBackend? Anyone have an idea of how sending this lineage data to the LineageBackend would be accomplished?

potiuk

Could you please add support (and tests) for mulitple queries ?

eladkal

This PR seems to incorporate also #15128 (and #11350 )

A side note: I'm not sure why my suggestion in the other PR to explore using execute_string
was neglected #11350 (comment) but mentioning it here as a possible alternative to explore if finding it useful.

eladkal · 2021-04-26T17:42:10Z

airflow/providers/snowflake/hooks/snowflake.py

Related to point 1 - this problem (allow multiple queries) is not unique to Snowflake. This also applies to Presto and Trino so whatever solution that will be accepted here will most likely be copied to the other two providers as well.

airflow/providers/snowflake/hooks/snowflake.py

mobuchowski · 2021-04-27T13:04:49Z

@potiuk @eladkal added requested changes.

potiuk

LGTM. @eladkal - are you ok with it in the context of other dbs? I think any DBHook change will have to be done as part of 2.1 but maybe this is a good start to implement it "per operator" and then generalise the approach for 2.1 (and then we could release versions of providers that will be 2.1+ compatible only when we have the generalisation in 2.1- as we discussed that we will do at some point in time.

github-actions · 2021-04-27T18:43:29Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

airflow/providers/snowflake/example_dags/example_snowflake.py

github-actions · 2021-04-27T18:50:35Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

mobuchowski · 2021-04-28T09:13:52Z

@potiuk one more CI run? 🙂

Signed-off-by: Maciej Obuchowski <[email protected]>

mobuchowski · 2021-04-30T14:21:31Z

@potiuk @eladkal rebased on master. Can we merge it if CI will pass?

potiuk · 2021-04-30T15:29:47Z

🤞

eladkal · 2021-04-30T15:44:49Z

LGTM. @eladkal - are you ok with it in the context of other dbs? I think any DBHook change will have to be done as part of 2.1 but maybe this is a good start to implement it "per operator" and then generalise the approach for 2.1 (and then we could release versions of providers that will be 2.1+ compatible only when we have the generalisation in 2.1- as we discussed that we will do at some point in time.

It's OK to do it per operator as well

boring-cyborg · 2021-04-30T15:51:17Z

Awesome work, congrats on your first merged pull request!

mobuchowski requested review from mik-laj, potiuk and turbaszek as code owners April 26, 2021 15:47

boring-cyborg bot added area:providers provider:snowflake Issues related to Snowflake provider labels Apr 26, 2021

potiuk reviewed Apr 26, 2021

View reviewed changes

potiuk requested changes Apr 26, 2021

View reviewed changes

eladkal reviewed Apr 26, 2021

View reviewed changes

mobuchowski changed the title ~~Expose snowflake query_id in snowflake hook and operator~~ Expose snowflake query_id in snowflake hook and operator, support multiple statements in sql string Apr 27, 2021

potiuk approved these changes Apr 27, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 27, 2021

potiuk reviewed Apr 27, 2021

View reviewed changes

airflow/providers/snowflake/example_dags/example_snowflake.py Outdated Show resolved Hide resolved

mobuchowski added 5 commits April 30, 2021 15:46

Expose snowflake query_id in snowflake hook and operator

855417f

multiple queries support

1d58123

Signed-off-by: Maciej Obuchowski <[email protected]>

multiple queries support system test

f2cf859

Signed-off-by: Maciej Obuchowski <[email protected]>

snowflake_op_sql_multiple_stmts

c40f05e

cleanup

907611f

Signed-off-by: Maciej Obuchowski <[email protected]>

mobuchowski force-pushed the expose-snowflake-query-id branch from 5fc8d29 to 907611f Compare April 30, 2021 13:47

eladkal approved these changes Apr 30, 2021

View reviewed changes

potiuk merged commit c6be8b1 into apache:master Apr 30, 2021

eladkal mentioned this pull request May 7, 2021

Allow multiple statements executions and return info of executions SnowflakeHook #15128

Closed

grassten mentioned this pull request May 26, 2021

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

Closed

mik-laj mentioned this pull request Dec 21, 2021

SnowflakeHook should not store query_ids #20398

Closed

eladkal mentioned this pull request Mar 21, 2022

Update the redshift sql operator to accept multiple sql statements #22391

Closed

eladkal mentioned this pull request Apr 20, 2022

Handle batch queries in DbApiHook? #23112

Closed

1 task

Expose snowflake query_id in snowflake hook and operator, support multiple statements in sql string #15533

Expose snowflake query_id in snowflake hook and operator, support multiple statements in sql string #15533

Uh oh!

Conversation

mobuchowski commented Apr 26, 2021

Uh oh!

boring-cyborg bot commented Apr 26, 2021

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

eladkal Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mobuchowski Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

potiuk Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mobuchowski Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

potiuk Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

mobuchowski Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

sunkickr May 7, 2021

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

eladkal left a comment

Choose a reason for hiding this comment

Uh oh!

eladkal Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mobuchowski commented Apr 27, 2021

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

Uh oh!

github-actions bot commented Apr 27, 2021

Uh oh!

mobuchowski commented Apr 28, 2021

Uh oh!

mobuchowski commented Apr 30, 2021

Uh oh!

potiuk commented Apr 30, 2021

Uh oh!

eladkal commented Apr 30, 2021

Uh oh!

boring-cyborg bot commented Apr 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eladkal Apr 26, 2021 •

edited

Loading

potiuk Apr 26, 2021 •

edited

Loading

eladkal Apr 26, 2021 •

edited

Loading

potiuk left a comment •

edited

Loading