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.

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

[KED-2865] Make sql datasets use a singleton pattern for connection #1163

Merged
merged 19 commits into from
Feb 3, 2022

Conversation

lorenabalan
Copy link
Contributor

@lorenabalan lorenabalan commented Jan 18, 2022

Signed-off-by: lorenabalan lorena.balan@quantumblack.com

Description

Part of the story about Making Remove Datasets Use a Singleton Pattern. Splitting it into 2 PRs, one for SQL datasets, one for Spark dataset(s).

Development notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

lorenabalan added 2 commits January 18, 2022 15:41
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan self-assigned this Jan 24, 2022
Lorena Bălan added 3 commits January 24, 2022 10:56
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan changed the title [WIP][KED-2865] Make remote datasets use a singleton pattern [WIP][KED-2865] Make sql datasets use a singleton pattern for connection Jan 24, 2022
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan changed the title [WIP][KED-2865] Make sql datasets use a singleton pattern for connection [KED-2865] Make sql datasets use a singleton pattern for connection Jan 24, 2022
lorenabalan added 2 commits January 24, 2022 17:31
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan force-pushed the feat/singleton-connection branch from de3e7ae to 2af5c5f Compare January 27, 2022 15:44
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan force-pushed the feat/singleton-connection branch from 1b8b917 to d71ba14 Compare January 27, 2022 16:03
@lorenabalan lorenabalan marked this pull request as ready for review January 27, 2022 16:04
@lorenabalan lorenabalan requested a review from idanov as a code owner January 27, 2022 16:04
@datajoely
Copy link
Contributor

Really nice work

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Just left a few comments for now - I'm going to read through that GitHub issue and ponder a bit more but basically I think looks great 💯 ⭐

kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved
kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved
tests/extras/datasets/pandas/test_sql_dataset.py Outdated Show resolved Hide resolved
tests/extras/datasets/pandas/test_sql_dataset.py Outdated Show resolved Hide resolved
Copy link
Member

@idanov idanov left a comment

Choose a reason for hiding this comment

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

Looks quite nice, it needs some refactoring of the tests (as in your own comments suggest) and maybe look into create the class attribute similarly to the default arguments, but overall I think we'll be ready to merge this soon. Hopefully it makes it harder for people to run out of connections when having many SQL-based datasets in their pipelines.

kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved
kedro/extras/datasets/pandas/sql_dataset.py Show resolved Hide resolved
kedro/extras/datasets/pandas/sql_dataset.py Show resolved Hide resolved
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan force-pushed the feat/singleton-connection branch from fc20b4f to a19d3fd Compare February 2, 2022 13:18
lorenabalan added 5 commits February 2, 2022 16:55
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan force-pushed the feat/singleton-connection branch from 3e3c81c to 372cf29 Compare February 3, 2022 11:13
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

I like this a lot! Very elegant solution 🌟

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Really nice work, especially with the testing! ⭐

Copy link
Member

@idanov idanov left a comment

Choose a reason for hiding this comment

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

🎉

@lorenabalan lorenabalan merged commit ceae20c into main Feb 3, 2022
@lorenabalan lorenabalan deleted the feat/singleton-connection branch February 3, 2022 14:15
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
…edro-org#1163)

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
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.

5 participants