Generalise Backend Layer #604

varun-edachali-dbx · 2025-06-18T03:50:36Z

What type of PR is this?

Refactor

Description

Separate the Session functionality from the Connection class to further abstract backend implementation details from the connection class: Separate Session related functionality from Connection class #571
Introduce a general backend interface (DatabricksClient): Introduce Backend Interface (DatabricksClient) #573
- Introduce the DatabricksClient interface and make the existing ThriftBackend implement it as ThriftDatabricksClient.
- Introduce SessionId and CommandId as consistent adapters for backend layers to represent sessions and commands instead of relying on Thrift specific types.
Introduce a general ResultSet interface: Implement ResultSet Abstraction (backend interfaces for fetch phase) #574
- Create a ResultSet interface and make the existing ResultSet interface implement it as ThriftResultSet.
- Make the concrete Thrift backend return a ThriftResultSet instead of ExecuteResponse from execution relevant commands.
- Generalise the return types of methods in the backend interface to prevent alignment with Thrift (introduce CommandState to prevent using Thrift's status types).

How is this tested?

Unit tests
E2E Tests
Manually
N/A

Related Tickets & Documents

Design Doc

* decouple session class from existing Connection ensure maintenance of current APIs of Connection while delegating responsibility Signed-off-by: varun-edachali-dbx <[email protected]> * add open property to Connection to ensure maintenance of existing API Signed-off-by: varun-edachali-dbx <[email protected]> * update unit tests to address ThriftBackend through session instead of through Connection Signed-off-by: varun-edachali-dbx <[email protected]> * chore: move session specific tests from test_client to test_session Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) as in CONTRIBUTING.md Signed-off-by: varun-edachali-dbx <[email protected]> * use connection open property instead of long chain through session Signed-off-by: varun-edachali-dbx <[email protected]> * trigger integration workflow Signed-off-by: varun-edachali-dbx <[email protected]> * fix: ensure open attribute of Connection never fails in case the openSession takes long, the initialisation of the session will not complete immediately. This could make the session attribute inaccessible. If the Connection is deleted in this time, the open() check will throw because the session attribute does not exist. Thus, we default to the Connection being closed in this case. This was not an issue before because open was a direct attribute of the Connection class. Caught in the integration tests. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: de-complicate earlier connection open logic earlier, one of the integration tests was failing because 'session was not an attribute of Connection'. This is likely tied to a local configuration issue related to unittest that was causing an error in the test suite itself. The tests are now passing without checking for the session attribute. c676f9b Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "fix: de-complicate earlier connection open logic" This reverts commit d6b1b19. Signed-off-by: varun-edachali-dbx <[email protected]> * [empty commit] attempt to trigger ci e2e workflow Signed-off-by: varun-edachali-dbx <[email protected]> * Update CODEOWNERS (#562) new codeowners Signed-off-by: varun-edachali-dbx <[email protected]> * Enhance Cursor close handling and context manager exception management to prevent server side resource leaks (#554) * Enhance Cursor close handling and context manager exception management * tests * fmt * Fix Cursor.close() to properly handle CursorAlreadyClosedError * Remove specific test message from Cursor.close() error handling * Improve error handling in connection and cursor context managers to ensure proper closure during exceptions, including KeyboardInterrupt. Add tests for nested cursor management and verify operation closure on server-side errors. * add * add Signed-off-by: varun-edachali-dbx <[email protected]> * PECOBLR-86 improve logging on python driver (#556) * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * PECOBLR-86 Improve logging for debug level Signed-off-by: Sai Shree Pradhan <[email protected]> * fixed format Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> * changed debug to error logs Signed-off-by: Sai Shree Pradhan <[email protected]> * used lazy logging Signed-off-by: Sai Shree Pradhan <[email protected]> --------- Signed-off-by: Sai Shree Pradhan <[email protected]> Signed-off-by: varun-edachali-dbx <[email protected]> * Revert "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit dbb2ec5, reversing changes made to 7192f11. Signed-off-by: varun-edachali-dbx <[email protected]> * Reapply "Merge remote-tracking branch 'upstream/sea-migration' into decouple-session" This reverts commit bdb8381. Signed-off-by: varun-edachali-dbx <[email protected]> * fix: separate session opening logic from instantiation ensures correctness of self.session.open call in Connection Signed-off-by: varun-edachali-dbx <[email protected]> * fix: use is_open attribute to denote session availability Signed-off-by: varun-edachali-dbx <[email protected]> * fix: access thrift backend through session Signed-off-by: varun-edachali-dbx <[email protected]> * chore: use get_handle() instead of private session attribute in client Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * fix: remove accidentally removed assertions Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]> Signed-off-by: Sai Shree Pradhan <[email protected]> Co-authored-by: Jothi Prakash <[email protected]> Co-authored-by: Madhav Sainanee <[email protected]> Co-authored-by: Sai Shree Pradhan <[email protected]>

NOTE: the `test_complex_types` e2e test was not working at the time of this merge. The test must be triggered when the test is back up and running as intended. * remove excess logs, assertions, instantiations large merge artifacts Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) + remove excess log (merge artifact) Signed-off-by: varun-edachali-dbx <[email protected]> * fix typing Signed-off-by: varun-edachali-dbx <[email protected]> * remove un-necessary check Signed-off-by: varun-edachali-dbx <[email protected]> * remove un-necessary replace call Signed-off-by: varun-edachali-dbx <[email protected]> * introduce __str__ methods for CommandId and SessionId Signed-off-by: varun-edachali-dbx <[email protected]> * docstrings for DatabricksClient interface Signed-off-by: varun-edachali-dbx <[email protected]> * stronger typing of Cursor and ExecuteResponse Signed-off-by: varun-edachali-dbx <[email protected]> * remove utility functions from backend interface, fix circular import Signed-off-by: varun-edachali-dbx <[email protected]> * rename info to properties Signed-off-by: varun-edachali-dbx <[email protected]> * newline for cleanliness Signed-off-by: varun-edachali-dbx <[email protected]> * fix circular import Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * to_hex_id -> get_hex_id Signed-off-by: varun-edachali-dbx <[email protected]> * better comment on protocol version getter Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * move guid to hex id to new utils module Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * move staging allowed local path to connection props Signed-off-by: varun-edachali-dbx <[email protected]> * add strong return type for execute_command Signed-off-by: varun-edachali-dbx <[email protected]> * skip auth, error handling in databricksclient interface Signed-off-by: varun-edachali-dbx <[email protected]> * chore: docstring + line width Signed-off-by: varun-edachali-dbx <[email protected]> * get_id -> get_guid Signed-off-by: varun-edachali-dbx <[email protected]> * chore: docstring Signed-off-by: varun-edachali-dbx <[email protected]> * fix: to_hex_id -> to_hex_guid Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]>

…574) * ensure backend client returns a ResultSet type in backend tests Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * newline for cleanliness Signed-off-by: varun-edachali-dbx <[email protected]> * fix circular import Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * to_hex_id -> get_hex_id Signed-off-by: varun-edachali-dbx <[email protected]> * better comment on protocol version getter Signed-off-by: varun-edachali-dbx <[email protected]> * formatting (black) Signed-off-by: varun-edachali-dbx <[email protected]> * stricter typing for cursor Signed-off-by: varun-edachali-dbx <[email protected]> * correct typing Signed-off-by: varun-edachali-dbx <[email protected]> * correct tests and merge artifacts Signed-off-by: varun-edachali-dbx <[email protected]> * remove accidentally modified workflow files remnants of old merge Signed-off-by: varun-edachali-dbx <[email protected]> * chore: remove accidentally modified workflow files Signed-off-by: varun-edachali-dbx <[email protected]> * add back accidentally removed docstrings Signed-off-by: varun-edachali-dbx <[email protected]> * clean up docstrings Signed-off-by: varun-edachali-dbx <[email protected]> * log hex Signed-off-by: varun-edachali-dbx <[email protected]> * remove unnecessary _replace call Signed-off-by: varun-edachali-dbx <[email protected]> * add __str__ for CommandId Signed-off-by: varun-edachali-dbx <[email protected]> * take TOpenSessionResp in get_protocol_version to maintain existing interface Signed-off-by: varun-edachali-dbx <[email protected]> * active_op_handle -> active_mmand_id Signed-off-by: varun-edachali-dbx <[email protected]> * ensure None returned for close_command Signed-off-by: varun-edachali-dbx <[email protected]> * account for ResultSet return in new pydocs Signed-off-by: varun-edachali-dbx <[email protected]> * pydoc for types Signed-off-by: varun-edachali-dbx <[email protected]> * move common state to ResultSet aprent Signed-off-by: varun-edachali-dbx <[email protected]> * stronger typing in resultSet behaviour Signed-off-by: varun-edachali-dbx <[email protected]> * remove redundant patch in test Signed-off-by: varun-edachali-dbx <[email protected]> * add has_been_closed_server_side assertion Signed-off-by: varun-edachali-dbx <[email protected]> * remove redundancies in tests Signed-off-by: varun-edachali-dbx <[email protected]> * more robust close check Signed-off-by: varun-edachali-dbx <[email protected]> * use normalised state in e2e test Signed-off-by: varun-edachali-dbx <[email protected]> * simplify corrected test Signed-off-by: varun-edachali-dbx <[email protected]> * add line gaps after multi-line pydocs for consistency Signed-off-by: varun-edachali-dbx <[email protected]> * use normalised CommandState type in ExecuteResponse Signed-off-by: varun-edachali-dbx <[email protected]> --------- Signed-off-by: varun-edachali-dbx <[email protected]>

Signed-off-by: varun-edachali-dbx <[email protected]>

…nd forward refs, remove some unused imports Signed-off-by: varun-edachali-dbx <[email protected]>

src/databricks/sql/backend/databricks_client.py

src/databricks/sql/backend/thrift_backend.py

jprakash-db · 2025-06-24T10:23:59Z

src/databricks/sql/backend/thrift_backend.py

@@ -938,58 +991,106 @@ def execute_command(

        if async_op:
            self._handle_execute_response_async(resp, cursor)
+            return None


Can we have a EmptyResultSet instead of none cc @jayantsing-db

In the existing implementation (i.e. the main branch), nothing (None) is returned for async queries. Returning an EmptyResultSet would cause a change to user facing functionality, right?

src/databricks/sql/backend/thrift_backend.py

tests/unit/test_session.py

jprakash-db · 2025-06-24T11:02:53Z

src/databricks/sql/utils.py

@@ -26,6 +26,7 @@
    TSparkRowSetType,


Can you move the utils.py file into the utils folder and probably give a new name to it

I agree that it should be renamed, does queue.py sound fine?

I think it should remain in the existing directory though. The Connection, Cursor and ResultSet definitions are in the existing directory, so I think the Queue definitions should remain here too.
Also, if you were referring to backend/utils by utils folder above, then I don't think this should move there because this is not backend specific.

cc @jayantsing-db

src/databricks/sql/session.py

jprakash-db · 2025-06-24T11:05:52Z

src/databricks/sql/result_set.py

+        """Fetch all remaining rows as an Arrow table."""
+        pass
+
+    def close(self) -> None:


If you are creating an ABC leave all of them as ABC. Don't implement anything

Is there any reason for that?

Abstract classes are allowed to have concrete methods, and in this case the close implementation only depends on state that is defined in ResultSet, so I think it belongs here. This can help reduce repetition.

cc @jayantsing-db

Signed-off-by: varun-edachali-dbx <[email protected]>

…nd Cursor Signed-off-by: varun-edachali-dbx <[email protected]>

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx and others added 4 commits June 18, 2025 03:47

remove un-necessary initialisation assertions

450b80d

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 18, 2025 03:50 — with GitHub Actions Failure

remove un-necessary line break s

a926f02

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 18, 2025 03:56 — with GitHub Actions Failure

varun-edachali-dbx added 2 commits June 18, 2025 03:57

more un-necessary line breaks

55ad001

Signed-off-by: varun-edachali-dbx <[email protected]>

constrain diff of test_closing_connection_closes_commands

fa15730

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 04:13 — with GitHub Actions Inactive

reduce diff of test_closing_connection_closes_commands

019c7fb

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 18, 2025 04:15 — with GitHub Actions Failure

use pytest-like assertions for test_closing_connection_closes_commands

726abe7

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 04:18 — with GitHub Actions Inactive

ensure command_id is not None

bf6d41c

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 05:46 — with GitHub Actions Inactive

line breaks after multi-line pyfocs

5afa733

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 05:57 — with GitHub Actions Inactive

ensure non null operationHandle for commandId creation

e3dfd36

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 18, 2025 06:05 — with GitHub Actions Failure

use command_id methods instead of explicit guid_to_hex_id conversion

63360b3

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 06:11 — with GitHub Actions Inactive

remove un-necessary artifacts in test_session, add back assertion

13ffb8d

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 18, 2025 06:22 — with GitHub Actions Failure

varun-edachali-dbx marked this pull request as ready for review June 18, 2025 06:28

varun-edachali-dbx requested review from deeksha-db, samikshya-db, jprakash-db, jackyhu-db and madhav-db as code owners June 18, 2025 06:28

varun-edachali-dbx requested review from gopalldb, jayantsing-db, vikrantpuppala and shivam2680 as code owners June 18, 2025 06:28

varun-edachali-dbx temporarily deployed to azure-prod June 18, 2025 08:13 — with GitHub Actions Inactive

add from __future__ import annotations to remove string literals arou…

d759050

…nd forward refs, remove some unused imports Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx had a problem deploying to azure-prod June 19, 2025 11:07 — with GitHub Actions Failure

varun-edachali-dbx temporarily deployed to azure-prod June 19, 2025 11:44 — with GitHub Actions Inactive

jprakash-db reviewed Jun 24, 2025

View reviewed changes

varun-edachali-dbx added 8 commits June 24, 2025 11:21

move docstring of DatabricksClient within class

1e21434

Signed-off-by: varun-edachali-dbx <[email protected]>

move ThriftResultSet import to top of file

cd4015b

Signed-off-by: varun-edachali-dbx <[email protected]>

make backend/utils __init__ file empty

ed8b610

Signed-off-by: varun-edachali-dbx <[email protected]>

use from __future__ import annotations to remove string literals arou…

94d951e

…nd Cursor Signed-off-by: varun-edachali-dbx <[email protected]>

use lazy logging

c20058e

Signed-off-by: varun-edachali-dbx <[email protected]>

replace getters with property tag

fe3acb1

Signed-off-by: varun-edachali-dbx <[email protected]>

Merge branch 'main' into backend-refactors

9fb6a76

set active_command_id to None, not active_op_handle

61dfc4d

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 24, 2025 15:33 — with GitHub Actions Inactive

align test_session with pytest instead of unittest

64fb9b2

Signed-off-by: varun-edachali-dbx <[email protected]>

varun-edachali-dbx temporarily deployed to azure-prod June 24, 2025 15:40 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Generalise Backend Layer #604

Generalise Backend Layer #604

Uh oh!

varun-edachali-dbx commented Jun 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Uh oh!

varun-edachali-dbx Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Uh oh!

varun-edachali-dbx Jun 24, 2025

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Uh oh!

varun-edachali-dbx Jun 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

Generalise Backend Layer #604

Are you sure you want to change the base?

Generalise Backend Layer #604

Uh oh!

Conversation

varun-edachali-dbx commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

Uh oh!

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

varun-edachali-dbx Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

varun-edachali-dbx Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jprakash-db Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

varun-edachali-dbx Jun 24, 2025 • 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!

varun-edachali-dbx commented Jun 18, 2025 •

edited

Loading

varun-edachali-dbx Jun 24, 2025 •

edited

Loading