Databricks SQL Sensor #30204

harishkrao · 2023-03-20T23:14:56Z

Databricks Delta table sensor.

harishkrao · 2023-03-20T23:16:21Z

@alexott @josh-fell @Taragolis @eladkal @o-nikolas as I mentioned in #28950 I have opened a new PR to divide the changes. The current PR is for Databricks SQL Sensor only.
It would be great if you can review the changes. Thank you!

airflow/providers/databricks/sensors/sql.py

o-nikolas · 2023-03-21T05:21:34Z

tests/system/providers/databricks/example_databricks_sensor.py

What are these tags for?

They were not needed, removed them.

o-nikolas · 2023-03-21T05:23:30Z

tests/system/providers/databricks/example_databricks_sensor.py

Can you add a doc file (which would use this tag)?

Added doc md to the DAG object in the example.

Hey @harishkrao,

Sorry, I should have been more explicit! Those tags are meant to be used in doc readmes for each operator. You can see an example here: https://github.com/apache/airflow/blob/main/docs/apache-airflow-providers-amazon/operators/sqs.rst#sensors

There already exists a doc readme for Databricks sql Operator (link), you can just update that doc to include a section for the Sensor.

Thank you for the examples, let me add the section.

@o-nikolas Added the section, please let me know if it looks good. Also, would be great if you can review once again. I've pushed all the changes from the feedback.

Looking good! The only thing missing is that you have doc tags for setting up the connection (howto_sensor_databricks_connection_setup) but you don't reference them from the doc. Can you either reference them in the doc you added, or remove the tags from this example dag.

Thanks for catching that, I will add it to the doc.

harishkrao · 2023-03-22T06:02:34Z

@o-nikolas pushed changes reflecting PR comments, can you please review once more?

alexott

looks good to me

alexott · 2023-03-25T14:10:01Z

airflow/providers/databricks/sensors/sql.py

I'm still not sure that for sensor we need to use fetch_all_handler

airflow/providers/databricks/sensors/sql.py

josh-fell · 2023-03-27T18:45:07Z

tests/system/providers/databricks/example_databricks_sensor.py

Suggested change

(sql_sensor)

from tests.system.utils.watcher import watcher

# This test needs watcher in order to properly mark success/failure

# when "tearDown" task with trigger rule is part of the DAG

list(dag.tasks) >> watcher()

The (sql_sensor) expression doesn't look necessary. Also, you don't the watcher() since this DAG only has 1 task and there are no tasks which are considered "teardown" tasks.

Agreed, will remove it.

A fair callout Josh! Though, I'd personally suggest against that optimization. I think it's helpful for all system tests to follow the same pattern so that they have a familiar look and feel. It also makes it easier for this test to be expanded in the future without someone having to go dig up what the rest of the pattern is if you've removed it (if they even know to do so!).

Completely agree consistency is ideal, but not all system tests would need, or even have a valid use case for, a "teardown" task; just adds a redundant task to the system test. Maybe it's the comment for the "need" for the watcher that makes this more confusing for me.

Not a hill to die on of course. I'll let @harishkrao and @alexott run with the suggestion if they choose. All good with me.

I suppose a compromise could be to remove the comment. But I also agree that it's certainly no hill to die on! It's not the end of the world either way, happy to leave it up to Harish :)

I agree with both points -- consistency and the point about not having the watcher for this example since it doesn't need it.
I have pushed a change to keep the watcher as-is for consistency and as you pointed out @josh-fell, the comment above it is confusing, indicating that the watcher is mandatory, so I have re-worded it.
Same applies to the (sql_sensor), I have added that this is not needed, but is present here as an extensible pattern.

josh-fell · 2023-03-27T18:53:21Z

tests/providers/databricks/sensors/test_sql.py

WDYT about adding a test for the interaction between http_path and sql_endpoint_name based on the sensor docstring:

:param http_path: Optional string specifying HTTP path of Databricks SQL Endpoint or cluster. **If not specified, it should be either specified in the Databricks connection's extra parameters, or ``sql_endpoint_name`` must be specified.** :param sql_endpoint_name: Optional name of Databricks SQL Endpoint. **If not specified, ``http_path`` must be provided as described above**, defaults to None ...

Sure, let me add it. Thank you.

@josh-fell I have addressed other items. I am working on adding unit tests for the above use case. I will push the change soon.

Co-authored-by: Niko Oliveira <onikolas@amazon.com>

Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

janvandervegt-db · 2023-03-29T08:47:50Z

airflow/providers/databricks/sensors/sql.py

+    :param http_path: Optional string specifying HTTP path of Databricks SQL Endpoint or cluster.
+        If not specified, it should be either specified in the Databricks connection's
+        extra parameters, or ``sql_endpoint_name`` must be specified.
+    :param sql_endpoint_name: Optional name of Databricks SQL Endpoint. If not specified, ``http_path``


Nit but I assume the "sql_endpoint_name" is going to be a more common and intuitive setting, does it make sense to define it above the "http_path"?

bilalaslamseattle · 2023-03-28T12:13:39Z

airflow/providers/databricks/sensors/sql.py

This reads wrong because SQL statements aren't really run on Delta tables ...? Rephrase as: "Sensor that runs a SQL query on Databricks"

bilalaslamseattle · 2023-03-28T12:13:53Z

airflow/providers/databricks/sensors/sql.py

SQL Warehouse, not endpoint.

All Purpose cluster not cluster.

bilalaslamseattle · 2023-03-28T12:14:22Z

airflow/providers/databricks/sensors/sql.py

Please check sql_endpoint_name parameter ... can this be better named as sql_warehouse_name?

bilalaslamseattle · 2023-03-28T12:14:40Z

airflow/providers/databricks/sensors/sql.py

What happens when you use a SQL Warehouse and specify Spark session params?

bilalaslamseattle · 2023-03-28T12:14:53Z

airflow/providers/databricks/sensors/sql.py

I would spell out Databricks Runtime

bilalaslamseattle · 2023-03-28T12:15:12Z

docs/apache-airflow-providers-databricks/operators/sql.rst

bilalaslamseattle · 2023-03-28T12:15:29Z

tests/providers/databricks/sensors/test_sql.py

Maybe explicitly make this PERSONAL_ACCESS_TOKEN?

bilalaslamseattle · 2023-03-28T12:15:36Z

tests/providers/databricks/sensors/test_sql.py

boring-cyborg bot added area:providers area:system-tests provider:databricks labels Mar 20, 2023

o-nikolas reviewed Mar 21, 2023

View reviewed changes

harishkrao force-pushed the db-sql-sensor branch from 8b08ff2 to f8867e3 Compare March 22, 2023 03:38

harishkrao force-pushed the db-sql-sensor branch 4 times, most recently from 13119b0 to 53ba9f8 Compare March 25, 2023 05:36

alexott approved these changes Mar 25, 2023

View reviewed changes

airflow/providers/databricks/sensors/sql.py Outdated

Copy link

Contributor

alexott Mar 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure that for sensor we need to use fetch_all_handler

josh-fell reviewed Mar 27, 2023

View reviewed changes

harishkrao force-pushed the db-sql-sensor branch 2 times, most recently from 9b06a17 to c225a9a Compare March 28, 2023 16:11

harishkrao and others added 15 commits March 28, 2023 12:09

Databricks SQL Sensor

901da47

Databricks SQL Sensor

28224c7

Consolidate _get_results and _sql_sensor methods

ab8171f

Co-authored-by: Niko Oliveira <onikolas@amazon.com>

PR comments

6fb34ef

Added type for sql

e685c8e

Added type for sql

695d6e9

Added functools

4cb1a95

Added functools

bcbf072

Documentation example

aacfe55

Documentation example

91ee04d

Added documentation for example DAG

c85abe6

Added documentation for example DAG

1d99cdf

Fixed documentation in example DAG

6542180

Added doc md

478ece5

Added doc md

cdc758d

harishkrao and others added 5 commits March 28, 2023 12:09

Added documentation to readme file

36cd2a0

Added documentation to readme file

6a34c2c

Removed space

d06daa5

Align param directives.

b2c7619

Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

Added clarification comments

f590087

harishkrao force-pushed the db-sql-sensor branch from 185bcd1 to f590087 Compare March 28, 2023 19:09

janvandervegt-db reviewed Mar 29, 2023

View reviewed changes

bilalaslamseattle reviewed Mar 29, 2023

View reviewed changes

harishkesavarao mentioned this pull request Apr 2, 2023

Databricks SQL Sensor #30428

Closed

harishkrao closed this Apr 3, 2023

-    (sql_sensor)
-    from tests.system.utils.watcher import watcher
-    # This test needs watcher in order to properly mark success/failure
-    # when "tearDown" task with trigger rule is part of the DAG
-    list(dag.tasks) >> watcher()

Databricks SQL Sensor #30204

Databricks SQL Sensor #30204

Uh oh!

Conversation

harishkrao commented Mar 20, 2023

Uh oh!

harishkrao commented Mar 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harishkrao commented Mar 22, 2023

Uh oh!

alexott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!