Make databricks sql hook return a namedtuple #36205

Joffreybvn · 2023-12-13T11:47:54Z

This is a fix proposal for #36161 (review)

The ADR for Airflow's DB API specifies that DbApiHooks needs to return a tuple / list of tuples. The databricks hook by defaults returns a dict-like databricks.Row object which we now transform into a new nametuple - which is compliant with the 'common data structure' specification.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

potiuk

Looks good

potiuk · 2023-12-13T12:34:28Z

@Joffreybvn -> do you have more things to add to it or you think it's ready to review ?

airflow/providers/databricks/hooks/databricks_sql.py

phanikumv

We need to address #36205 (comment)

bolkedebruin

Given all the discussions around this earlier I'm a bit picky on the naming. We are not making something JSON serializable - but according to the earlier discussion we make the SQL hook return the specified NamedTuple format.

I wonder btw if we should not have a typedef for it if this is part the the DbAPI. That would make things conceptually stronger and less ambiguous.

If you insist on naming it something serializable please leave out JSON. It has nothing to do with it.

Can we further be clear on what this PR wants to achieve in the commit message? The title and message are now confusing.

My suggestion:

Return SerializableRow in databricks sql hook

The ADR for Airflow' s DB API specifies it needs to return a 
named tuple SerialzableRow or a list of them.  The databricks
hook by defaults returns a dict which we now transform to a
SerializableRow.

airflow/providers/databricks/hooks/databricks_sql.py

potiuk

I like the way it is now.

potiuk · 2023-12-14T14:29:03Z

@bolkedebruin @phanikumv -> please take a look and see what you think. I am all for this direction

potiuk · 2023-12-14T14:37:22Z

And I created #36224 and assigned to myself and will shortly take a close look at this - to make sure the common.sql API is not only defined but also enforced by the pre-commit.

When I started it last year and defined .pyi for common.sql to experiment with it, there was not much interest but seems that it's a good idea to finish the work as it causes contentions and issues like this one.

Also I hope it will become a golden standard for other common interfaces exposed to our providers where ADRs and static check enforecement on backwards compatibility will be followed.

Joffreybvn · 2023-12-14T14:49:06Z

The databricks provider was already released with the breaking change-> 5.0.1. Maybe this version should be removed / replaced by a new "5.0.1" one ?

potiuk · 2023-12-14T14:53:26Z

The databricks provider was already released with the breaking change-> 5.0.1. Maybe this version should be removed / replaced by a new "5.0.1" one ?

Nope. We can at most yank it. There is no way to replace existing released version. Luckily it has not been yet in constraints of any Airflow version, so we can do it without any serious consequences. Let me actually yank it now.

potiuk · 2023-12-14T14:57:32Z

Yanked

potiuk · 2023-12-14T15:01:47Z

Also cc: @ephraimbuddy -> since we yanked 5.0.1, I will rerun the last build for v2-8-stable to refresh constraints and move the tag once done (currentlly 5.0.1 was in the constraints planned to release for 2.8.0rc3)

Job running here: https://github.com/apache/airflow/actions/runs/7199187385

airflow/providers/databricks/hooks/databricks_sql.py

potiuk · 2023-12-14T16:42:16Z

Constraints with databricks 5.0.0 updated 3312193 and tag for 2.8.0rc3 moved @ephraimbuddy

airflow/providers/databricks/hooks/databricks_sql.py

utkarsharma2 · 2023-12-16T10:09:11Z

@bolkedebruin For a long-term solution would it make sense to have a way for a provider to register a custom serializer/deserializer for objects its operator might produce? We can extend this logic to include providers.

…r clarity Apply the ADR 0002 - https://github.com/apache/airflow/blob/main/airflow/providers/common/sql/doc/adr/0002-return-common-data-structure-from-dbapihook-derived-hooks.md

…object

…in static checks

…ricks hooks

Joffreybvn · 2023-12-21T20:19:33Z

airflow/providers/databricks/hooks/databricks_sql.py

@@ -167,7 +195,7 @@ def run(
        handler: Callable[[Any], T] = ...,
        split_statements: bool = ...,
        return_last: bool = ...,
-    ) -> T | list[T]:
+    ) -> tuple | list[tuple] | list[list[tuple] | tuple] | None:


Mypy was not happy with -> tuple | list[tuple] | list[list[tuple] | tuple] as output - which is correct. I got error: "Overloaded function signatures 1 and 2 overlap with incompatible return types".

Following mypy docs, I added a None for it to be stop complaining. But that's not correct, this method cannot return None when a handler is provided...

Implementing definitive typing is out of the scope of this PR, and I admit mypy beat me on that case. Thus I let that for #36224 - but I'm whiling to help on that issue too of course.

Joffreybvn · 2023-12-21T20:25:49Z

To be honest, I found quite interesting to be in controversial PR. Thus, totally fine for me :)

Current status:

Refactored _make_common_data_structure with the suggested typing, without None.
No more complex instance check, this function assume to have Row as input
Little hack to make mypy happy here, but let's fix that definitively with @potiuk's issue Make the interface of common.sql DBApiHook type-defined #36224 ?

bolkedebruin

Much much better! I understand what this is about now. The follow up would make things even clearer so absolutely appreciated if you want to help out there.

potiuk · 2023-12-23T00:40:39Z

Hey @bolkedebruin -> next time when you merge PR - can you please make sure you keep "squash and merge" for PRs and do not remove the number - this one has been merged without it and it has no (#36205) in the name - which made it disappear from our release notes generation. I noticed it accidentally luckily.

potiuk · 2023-12-23T00:51:03Z

bolkedebruin · 2023-12-23T17:42:32Z

Sorry @potiuk i did actually squash and merge but adjusted the commit message to cover what was done. That removed the ref. I'll be more careful

potiuk · 2023-12-23T19:08:13Z

No worries :). I caught it luckily

boring-cyborg bot added area:providers provider:databricks labels Dec 13, 2023

Joffreybvn marked this pull request as draft December 13, 2023 11:48

potiuk approved these changes Dec 13, 2023

View reviewed changes

utkarsharma2 reviewed Dec 13, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks_sql.py Outdated Show resolved Hide resolved

Joffreybvn marked this pull request as ready for review December 13, 2023 13:12

phanikumv requested changes Dec 13, 2023

View reviewed changes

potiuk mentioned this pull request Dec 13, 2023

Status of testing Providers that were prepared on December 12, 2023 #36194

Closed

19 tasks

bolkedebruin requested changes Dec 13, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks_sql.py Outdated Show resolved Hide resolved

bolkedebruin changed the title ~~Databricks sql hook returns json-serializable namedtuple~~ Make databricks sql hook return a serializable namedtuple Dec 13, 2023

Joffreybvn requested a review from eladkal as a code owner December 13, 2023 20:39

Joffreybvn force-pushed the fix/make-databricks-returns-namedtuples branch from cffce3e to cde35bb Compare December 13, 2023 21:38

potiuk approved these changes Dec 14, 2023

View reviewed changes

potiuk requested a review from bolkedebruin December 14, 2023 14:27

bolkedebruin reviewed Dec 14, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks_sql.py Outdated Show resolved Hide resolved

Joffreybvn force-pushed the fix/make-databricks-returns-namedtuples branch from f3619e2 to 8b1b0e6 Compare December 14, 2023 20:26

bolkedebruin reviewed Dec 15, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks_sql.py Outdated Show resolved Hide resolved

bolkedebruin reviewed Dec 15, 2023

View reviewed changes

airflow/providers/databricks/hooks/databricks_sql.py Outdated Show resolved Hide resolved

Joffreybvn marked this pull request as draft December 16, 2023 15:38

Joffreybvn force-pushed the fix/make-databricks-returns-namedtuples branch from 9255d9a to 2bfb8ad Compare December 16, 2023 18:42

joffreybienvenu-infrabel added 16 commits December 21, 2023 19:00

fix: docstring spellchecks

7c00732

fix: rewrite docstring and warning message

eceec0b

fix: docstring spellchecks

67d1b90

feat: rename make_serializable into make_common_data_structure fo…

1e84a75

…r clarity Apply the ADR 0002 - https://github.com/apache/airflow/blob/main/airflow/providers/common/sql/doc/adr/0002-return-common-data-structure-from-dbapihook-derived-hooks.md

feat: Add typing for _make_common_data_structure in databricks

e76d5b8

feat: Apply typing to ODBC hook

a82553e

fix: patch pyodbc.Row to make isinstance() checks pass with row_mock …

d3510ef

…object

fix: Use List in type casting for python38 compatibility

0f20dfb

feat: Remove all mentions of serialization in databricks hook

ab87191

feat: Rename _make_serializable into _make_common_data_structure …

10e22c7

…in static checks

fix: use return_tuple in databricks tests

4796882

fix: add deprecated _make_serializable method in odbc.py

61255ed

bump: min sql.common to 1.9.1 for odbc and databricks

e54ca54

fix: add version 1.9.1 to provider.yaml of common.sql

e0c7ad4

fix: move back-compat in common.sql

74a763d

feat: Implement strict tuple/list[tuple] return in ODBC and Datab…

affd2f4

…ricks hooks

Joffreybvn force-pushed the fix/make-databricks-returns-namedtuples branch from 49b8089 to affd2f4 Compare December 21, 2023 18:00

potiuk requested a review from bolkedebruin December 21, 2023 19:11

potiuk approved these changes Dec 21, 2023

View reviewed changes

Joffreybvn commented Dec 21, 2023

View reviewed changes

bolkedebruin approved these changes Dec 22, 2023

View reviewed changes

bolkedebruin merged commit 5fe5d31 into apache:main Dec 22, 2023
78 checks passed

potiuk mentioned this pull request Dec 23, 2023

Status of testing Providers that were prepared on December 23, 2023 #36384

Closed

84 tasks

This was referenced Sep 17, 2024

Changed type hinting for handler function Ariel2400/airflow#1

Open

Changed type hinting for handler function #42275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make databricks sql hook return a namedtuple #36205

Make databricks sql hook return a namedtuple #36205

Joffreybvn commented Dec 13, 2023 •

edited

Loading

potiuk left a comment

potiuk commented Dec 13, 2023

phanikumv left a comment

bolkedebruin left a comment •

edited

Loading

potiuk left a comment

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023

Joffreybvn commented Dec 14, 2023

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023 •

edited

Loading

potiuk commented Dec 14, 2023

utkarsharma2 commented Dec 16, 2023 •

edited

Loading

Joffreybvn Dec 21, 2023 •

edited

Loading

Joffreybvn commented Dec 21, 2023

bolkedebruin left a comment

potiuk commented Dec 23, 2023 •

edited

Loading

potiuk commented Dec 23, 2023

bolkedebruin commented Dec 23, 2023

potiuk commented Dec 23, 2023

Make databricks sql hook return a namedtuple #36205

Make databricks sql hook return a namedtuple #36205

Conversation

Joffreybvn commented Dec 13, 2023 • edited Loading

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Dec 13, 2023

phanikumv left a comment

Choose a reason for hiding this comment

bolkedebruin left a comment • edited Loading

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023

Joffreybvn commented Dec 14, 2023

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023

potiuk commented Dec 14, 2023 • edited Loading

potiuk commented Dec 14, 2023

utkarsharma2 commented Dec 16, 2023 • edited Loading

Joffreybvn Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Joffreybvn commented Dec 21, 2023

bolkedebruin left a comment

Choose a reason for hiding this comment

potiuk commented Dec 23, 2023 • edited Loading

potiuk commented Dec 23, 2023

bolkedebruin commented Dec 23, 2023

potiuk commented Dec 23, 2023

Joffreybvn commented Dec 13, 2023 •

edited

Loading

bolkedebruin left a comment •

edited

Loading

potiuk commented Dec 14, 2023 •

edited

Loading

utkarsharma2 commented Dec 16, 2023 •

edited

Loading

Joffreybvn Dec 21, 2023 •

edited

Loading

potiuk commented Dec 23, 2023 •

edited

Loading