Add an option to load SQL queries from a file for SQLQueryDataSet #887

BenjaminLevyQB · 2021-09-09T15:22:17Z

Description

Adds a new filepath argument to the constructor of SQLQueryDataSet that allows the user to specify a file containing a SQL query instead of having to write the entire query in the catalog, allowing users to keep the catalog clean and take advantage of SQL syntax highlighting in the case of long queries

Development notes

Added filepath and fs_args to the constructor to allow fsspec-based loading of SQL query files
sql parameter is now optional, but an error is raised if neither sql nor filepath are supplied
Added test test_load_query_file and modified test_empty_query_error such that it raises the updated error

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 and added my name to the list of supporting contributions in the RELEASE.md file
Added tests to cover my changes

Notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

merelcht

Thanks for the contribution @BenjaminLevyQB ! Don't forget to update the RELEASE.md to include the changes as well as your name on the contributor list.

kedro/extras/datasets/pandas/sql_dataset.py

Co-authored-by: Merel Theisen <[email protected]>

…jaminLevyQB/kedro into feature/sql-dataset-filepath

antonymilne

Thank you for your excellent contribution! ⭐ I like this feature a lot but have two little concerns:

Is this technically a breaking change? The signature for SQLQueryDataSet has changed so that anyone calling it with positional arguments before (like SQLQueryDataSet("SELECT * FROM table", credentials) presumably won't work now. The principal use will be through catalog.yml, for which this doesn't matter, but I think anyone using the Python API will suffer a breaking change. Curious if others agree here - if so then this should go to develop rather than master
What happens if a user specifies both sql and filepath? Currently it looks like filepath will take precedent, but maybe this should throw an error message instead.

RELEASE.md

kedro/extras/datasets/pandas/sql_dataset.py

datajoely · 2021-09-29T14:40:45Z

What's left on this PR - I think it should be ready to go right? We should close #886 for reasons outlined here

BenjaminLevyQB · 2021-10-12T19:58:43Z

@datajoely looks good to me -- sorry about the delay, can I get a review on this so it can be merged?

datajoely · 2021-10-13T13:55:54Z

@BenjaminLevyQB I think there a few comments from @AntonyMilneQB (who is now on parental leave) to address

Co-authored-by: Antony Milne <[email protected]>

BenjaminLevyQB · 2021-10-13T18:13:57Z

Updated in response to @AntonyMilneQB's comments:

If both sql and filepath are given, an error is raised (instead of defaulting silently to sql)
filepath is added to the end instead of in the middle of the arguments. This keeps sql being changed to a keyword argument, but now if the arguments are specified positionally (e.g., through the python API) then anyone who did not know about filepath previously will still have their code work.

…to feature/sql-dataset-filepath

BenjaminLevyQB · 2021-10-19T17:44:10Z

Can I get a review on the updated code? Thanks!

antonymilne · 2021-10-20T10:38:39Z

Hi @BenjaminLevyQB, thanks for your updates on this and apologies for the delayed review - I just got back from leave. Moving filepath to the end of the arguments is a good idea but I'm not quite sure whether it's the right thing to do here in order to avoid a breaking change. Let me check with someone else on the team to see what they think and then we can get this merged 🙂 Have discussed this and it's fine - good solution for backwards compatibility!

lorenabalan

Very nice feature addition! Left a few suggestions, let me know what you think. 😊

tests/extras/datasets/pandas/test_sql_dataset.py

kedro/extras/datasets/pandas/sql_dataset.py

lorenabalan · 2021-10-21T16:39:28Z

kedro/extras/datasets/pandas/sql_dataset.py

+                Here you can find all available arguments for `open`:
+                https://filesystem-spec.readthedocs.io/en/latest/api.html#fsspec.spec.AbstractFileSystem.open
+                All defaults are preserved, except `mode`, which is set to `r` when loading
+                and to `w` when saving.


Suggested change

and to `w` when saving.

and to `w` when saving.

This can be removed, we don't actually set mode to w ourselves anymore.

kedro/extras/datasets/pandas/sql_dataset.py

lorenabalan · 2021-10-21T16:50:25Z

kedro/extras/datasets/pandas/sql_dataset.py


    def _load(self) -> pd.DataFrame:
+        load_args = self._load_args.copy()


Doesn't this return a shallow copy instead of deepcopy? I think the latter is the one we want for dictionaries generally. 🤔

kedro/extras/datasets/pandas/sql_dataset.py

lorenabalan · 2021-10-21T16:55:23Z

kedro/extras/datasets/pandas/sql_dataset.py


    def _load(self) -> pd.DataFrame:
+        load_args = self._load_args.copy()
+
+        if "sql" not in load_args:


Any reason why not if "filepath" in load_args? 😅 Or if self._filepath if we go with the previous suggestion.

kedro/extras/datasets/pandas/sql_dataset.py

datajoely · 2021-10-25T09:25:56Z

@BenjaminLevyQB amazing work so far - some little bits needed before we can get this in. I want to release 0.17.6 shortly so this would be a wonderful addition.

…jaminLevyQB/kedro into feature/sql-dataset-filepath

BenjaminLevyQB · 2021-10-25T14:01:23Z

Thank you @lorenabalan and @AntonyMilneQB for you excellent comments! The requested changes have been made!!

antonymilne

Thanks very much for all your work on this @BenjaminLevyQB. I'll make it sure it gets merged 🙂

kedro/extras/datasets/pandas/sql_dataset.py

tests/extras/datasets/pandas/test_sql_dataset.py

…dro-org#887) Signed-off-by: Laurens Vijnck <[email protected]>

creating sql_dataset branch

dc8a7e7

This was referenced Sep 9, 2021

Adding sql file support for SQLQueryDataSet and creating new SQLConnectionDataSet class #879

Closed

Improved SQL functionality #880

Closed

idanov changed the title ~~creating sql_dataset branch~~ Add an option to load SQL queries from a file for SQLQueryDataSet Sep 9, 2021

Merge branch 'master' into feature/sql-dataset-filepath

ad95db3

BenjaminLevyQB marked this pull request as ready for review September 9, 2021 19:52

BenjaminLevyQB requested a review from idanov as a code owner September 9, 2021 19:52

merelcht approved these changes Sep 10, 2021

View reviewed changes

kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved

kedro/extras/datasets/pandas/sql_dataset.py Show resolved Hide resolved

BenjaminLevyQB and others added 4 commits September 10, 2021 11:28

Update kedro/extras/datasets/pandas/sql_dataset.py

dd425af

Co-authored-by: Merel Theisen <[email protected]>

improving desc for sql dataset

7fce17a

Merge branch 'feature/sql-dataset-filepath' of https://github.com/Ben…

f5c95b8

…jaminLevyQB/kedro into feature/sql-dataset-filepath

Updating RELEASE.md

68e734e

antonymilne reviewed Sep 10, 2021

View reviewed changes

RELEASE.md Outdated Show resolved Hide resolved

RELEASE.md Outdated Show resolved Hide resolved

kedro/extras/datasets/pandas/sql_dataset.py Show resolved Hide resolved

kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved

datajoely mentioned this pull request Oct 11, 2021

Enhancements to SparkDataSet #945

Closed

Merge branch 'master' into feature/sql-dataset-filepath

19ab35a

BenjaminLevyQB and others added 4 commits October 13, 2021 11:40

Update RELEASE.md

fdfa53e

Co-authored-by: Antony Milne <[email protected]>

Update RELEASE.md

53dc89e

Co-authored-by: Antony Milne <[email protected]>

changed dataset to raise error if both sql and filepath are provided

a64089b

moved filepath argument so that this is no longer a breaking change

862fafc

BenjaminLevyQB and others added 2 commits October 13, 2021 14:14

Merge branch 'master' of https://github.com/quantumblacklabs/kedro in…

ebe91d6

…to feature/sql-dataset-filepath

Merge branch 'master' into feature/sql-dataset-filepath

6376fc2

lorenabalan reviewed Oct 21, 2021

View reviewed changes

datajoely reviewed Oct 24, 2021

View reviewed changes

kedro/extras/datasets/pandas/sql_dataset.py Outdated Show resolved Hide resolved

BenjaminLevyQB added 2 commits October 25, 2021 09:58

resolving small nits from PR conversation

6b48a3e

Merge branch 'feature/sql-dataset-filepath' of https://github.com/Ben…

0d7f627

…jaminLevyQB/kedro into feature/sql-dataset-filepath

Merge branch 'master' into feature/sql-dataset-filepath

ae65d53

antonymilne approved these changes Oct 25, 2021

View reviewed changes

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

antonymilne added 3 commits October 25, 2021 16:01

Update kedro/extras/datasets/pandas/sql_dataset.py

7c507b9

Update kedro/extras/datasets/pandas/sql_dataset.py

d36d998

Merge branch 'master' into feature/sql-dataset-filepath

c8cabce

antonymilne self-assigned this Oct 25, 2021

antonymilne added 2 commits October 25, 2021 16:26

Update tests/extras/datasets/pandas/test_sql_dataset.py

a90a1de

Merge branch 'master' into feature/sql-dataset-filepath

6979f25

antonymilne merged commit 982dc70 into kedro-org:master Oct 25, 2021

datajoely mentioned this pull request Nov 12, 2021

Allow custom SQL query for GBQTableDataSet #1032

Closed

antonymilne mentioned this pull request Nov 17, 2021

Support schema load argument for SparkDataset #986

Merged

6 tasks

lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022

Add an option to load SQL queries from a file for SQLQueryDataSet (ke…

58e97ac

…dro-org#887) Signed-off-by: Laurens Vijnck <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to load SQL queries from a file for SQLQueryDataSet #887

Add an option to load SQL queries from a file for SQLQueryDataSet #887

BenjaminLevyQB commented Sep 9, 2021

merelcht left a comment

antonymilne left a comment •

edited

Loading

datajoely commented Sep 29, 2021 •

edited

Loading

BenjaminLevyQB commented Oct 12, 2021

datajoely commented Oct 13, 2021 •

edited

Loading

BenjaminLevyQB commented Oct 13, 2021

BenjaminLevyQB commented Oct 19, 2021

antonymilne commented Oct 20, 2021 •

edited

Loading

lorenabalan left a comment

lorenabalan Oct 21, 2021

lorenabalan Oct 21, 2021

lorenabalan Oct 21, 2021

datajoely commented Oct 25, 2021

BenjaminLevyQB commented Oct 25, 2021

antonymilne left a comment


		def _load(self) -> pd.DataFrame:
		load_args = self._load_args.copy()

Add an option to load SQL queries from a file for SQLQueryDataSet #887

Add an option to load SQL queries from a file for SQLQueryDataSet #887

Conversation

BenjaminLevyQB commented Sep 9, 2021

Description

Development notes

Checklist

Notice

merelcht left a comment

Choose a reason for hiding this comment

antonymilne left a comment • edited Loading

Choose a reason for hiding this comment

datajoely commented Sep 29, 2021 • edited Loading

BenjaminLevyQB commented Oct 12, 2021

datajoely commented Oct 13, 2021 • edited Loading

BenjaminLevyQB commented Oct 13, 2021

BenjaminLevyQB commented Oct 19, 2021

antonymilne commented Oct 20, 2021 • edited Loading

lorenabalan left a comment

Choose a reason for hiding this comment

lorenabalan Oct 21, 2021

Choose a reason for hiding this comment

lorenabalan Oct 21, 2021

Choose a reason for hiding this comment

lorenabalan Oct 21, 2021

Choose a reason for hiding this comment

datajoely commented Oct 25, 2021

BenjaminLevyQB commented Oct 25, 2021

antonymilne left a comment

Choose a reason for hiding this comment

antonymilne left a comment •

edited

Loading

datajoely commented Sep 29, 2021 •

edited

Loading

datajoely commented Oct 13, 2021 •

edited

Loading

antonymilne commented Oct 20, 2021 •

edited

Loading