Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add ConnectionWrapper base class #828

Merged
merged 6 commits into from
Aug 10, 2023
Merged

Add ConnectionWrapper base class #828

merged 6 commits into from
Aug 10, 2023

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Jul 7, 2023 •
edited
Loading

resolves #829

Description

Checklist

@Fokko Fokko requested a review from a team as a code owner July 7, 2023 11:48
@Fokko Fokko requested a review from McKnight-42 July 7, 2023 11:48
@cla-bot cla-bot bot added the cla:yes label Jul 7, 2023
@JCZuurmond
Copy link
Collaborator

@Fleid: This PR is ready to be merged

@JCZuurmond
Copy link
Collaborator

@McKnight-42 : The CI's are failing unrelated to the code changes. We have seen that CI being flacky before. Could you investigate that?

@McKnight-42
Copy link
Contributor

@JCZuurmond no worries i can rekick off the failing tests and I'll keep and eye on it.

@colin-rogers-dbt
Copy link
Contributor

Looks like spark-session tests are failing due to:

>   from dbt.spark.adapters import ConnectionWrapper
E   ModuleNotFoundError: No module named 'dbt.spark'

/root/project/dbt/adapters/spark/session.py:12: ModuleNotFoundError

Probably just a missing dependency in our spark image

@JCZuurmond JCZuurmond mentioned this pull request Jul 18, 2023
Merged
6 tasks
mikealfare
mikealfare reviewed Jul 19, 2023
View reviewed changes
dbt/adapters/spark/connections.py Outdated
@@ -154,7 +156,38 @@ def _connection_keys(self) -> Tuple[str, ...]:
return "host", "port", "cluster", "endpoint", "schema", "organization"


class PyhiveConnectionWrapper(object):
class ConnectionWrapper(ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like this idea, but I have some questions. How does this class differ from BaseConnectionManager in dbt-core:
https://github.com/dbt-labs/dbt-core/blob/a1b067c683212bccd9bc10f6e52bcdd168ccf7ec/core/dbt/adapters/base/connections.py#L59
I know we already had PyhiveConnectionWrapper; I'm not very familiar with that class, but it also seems specific to dbt-spark. This interface looks very generic, like a set of minimal methods that any adapter would need to connect to something. Put another way, if we do need this class in addition to BaseConnectionManager, perhaps we should put ConnectionWrapper in dbt-core and inherit from it here? Is there any reason to not do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ConnectionWrapper is pretty spark specific for the time being, @Fokko in case we want to add a generic connection wrapper in core in the future can we rename this class to: SparkConnectionWrapper?

Suggested change
class ConnectionWrapper(ABC):
class SparkConnectionWrapper(ABC):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I agree with Colin, I think it is very Spark-specific, and I don't it doesn't add much value to move this to the core package. I've renamed the class 👍🏻

mikealfare
mikealfare reviewed Aug 10, 2023
View reviewed changes
dbt/adapters/spark/session.py Outdated
@@ -9,6 +9,7 @@
from dbt.events import AdapterLogger
from dbt.utils import DECIMALS
from pyspark.sql import DataFrame, Row, SparkSession
from dbt.spark.adapters import ConnectionWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this line and it's reference below need to be updated? I'm surprised this passed tests; normally we'd import from dbt.adapters.spark, and in this case dbt.adapters.spark.connections since this class isn't in dbt.adapters.spark.__init__.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably because we have the spark session tests disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure how this happened, and also a bit scary that the linter didn't catch this.

colin-rogers-dbt
colin-rogers-dbt requested changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt left a comment

Choose a reason for hiding this comment

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

Need to cascade changes to ConnectionWrapper to SessionConnectionWrapper

@Fokko
Copy link
Contributor Author

Fokko commented Aug 10, 2023

@mikealfare @colin-rogers-dbt I think my PR was in a limbo state, I didn't correctly push my local changes because it was stuck in a merge. After fixing that, mypy was vocal again, and I had to push a few more changes. @JCZuurmond PTAL

@colin-rogers-dbt colin-rogers-dbt merged commit 5ed503a into dbt-labs:main Aug 10, 2023
7 checks passed
@Fokko Fokko deleted the fd-add-base-class branch August 10, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikealfare mikealfare mikealfare left review comments

@colin-rogers-dbt colin-rogers-dbt colin-rogers-dbt approved these changes

@JCZuurmond JCZuurmond JCZuurmond approved these changes

@McKnight-42 McKnight-42 Awaiting requested review from McKnight-42 McKnight-42 is a code owner automatically assigned from dbt-labs/adapters

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ADAP-677] [Feature] Add a BaseClass to the ConnectionWrapper
5 participants
@Fokko @JCZuurmond @McKnight-42 @colin-rogers-dbt @mikealfare