Skip to content

Introduce Backend Interface (DatabricksClient) #573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 149 commits into from
May 30, 2025

Conversation

varun-edachali-dbx
Copy link
Collaborator

@varun-edachali-dbx varun-edachali-dbx commented May 26, 2025

What type of PR is this?

  • Refactor

Description

  • Introduce the DatabricksClient interface and make the existing thrift backend implement it. This allows the cursor to not be aware of the type of backend instantiated. Currently, we have to include some assertions in the ResultSet to ensure we have a ThriftDatabricksClient type, because the fetch-phase abstractions have not been implemented yet.
  • Introduce SessionId and CommandId interfaces to create a consistent adapter to be used to represent sessions and commands instead of relying on Thrift (or eventually, SEA) specific types.

How is this tested?

  • Unit tests
    • some of the existing unit tests were slightly altered to account for the introduction of the interface. No unit tests were removed or introduced.
  • E2E Tests
  • Manually
  • N/A

Related Tickets & Documents

https://docs.google.com/document/d/1Y-eXLhNqqhrMVGnOlG8sdFrCxBTN1GdQvuKG4IfHmo0/edit?usp=sharing
https://databricks.atlassian.net/browse/PECOBLR-440?atlOrigin=eyJpIjoiMTgzNGNiMDVkMGQ3NDM2Njg5OTRhZWQ1MGQ4Mjg1OWIiLCJwIjoiaiJ9

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Copy link

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

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

@jayantsing-db jayantsing-db left a comment

Choose a reason for hiding this comment

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

Thanks for changes and most importantly changes to tests. Some minor nit comments, rest LGTM

@jayantsing-db
Copy link
Contributor

please make a note of the failing integration test in commit message when pushing if it is unrelated to these changes.

Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
Signed-off-by: varun-edachali-dbx <[email protected]>
@samikshya-db
Copy link
Contributor

In the future, let's try to raise smaller size PRs typically within 700 lines of code changes including tests. This will help you get more reviews and more importantly more thorough reviews. Thanks.

@jayantsing-db
Copy link
Contributor

In the future, let's try to raise smaller size PRs typically within 700 lines of code changes including tests. This will help you get more reviews and more importantly more thorough reviews. Thanks.

Hey @samikshya-db, it's tricky to further scope this down. Otherwise, it was adding too much overhead.

@samikshya-db
Copy link
Contributor

@jayantsing-db I understand this would be harder for the initial set of SEA PRs. Even then, it is good to keep this in mind and try to break it down. Happy to brainstorm on this too.

@jayantsing-db
Copy link
Contributor

jayantsing-db commented May 30, 2025

Even then, it is good to keep this in mind and try to break it down. Happy to brainstorm on this too.

yes

I understand this would be harder for the initial set of SEA PRs

heads up, this and couple more are refactoring PRs to prepare for SEA changes. refactoring PRs are usually high in LOC because we have to make sure current set of tests do not break and so the related changes have to go in together. Even then @varun-edachali-dbx first session re-factoring PR is ~500

for SEA PR, LOC will be low as expected per PR.

@varun-edachali-dbx varun-edachali-dbx merged commit 400a8bd into sea-migration May 30, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.