feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

MinuraPunchihewa · 2024-10-11T11:28:45Z

Description

This PR adds the ExternalTableDataset to support interactions with external tables in Databricks (Unity Catalog).

Fixes #817

Development notes

The ExternalTableDataset has been implemented by extending the BaseTableDataset that was added here.

These changes have been tested,

Manually, by running the code against a Unity Catalog enabled workspace.
Via the existing and newly added unit tests.

Checklist

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 relevant RELEASE.md file
Added tests to cover my changes

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-11T12:04:12Z

Hey @noklam, @ankatiyar,
I've added the ExternalTableDataset as an experimental dataset here. Is there anything else I need to do here, like tests etc.?

noklam · 2024-10-11T12:19:52Z

@MinuraPunchihewa Tests are super welcomed, although we don't have a databricks cluster to run these tests unless there are ways to run this locally.

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-11T14:18:55Z

@MinuraPunchihewa Tests are super welcomed, although we don't have a databricks cluster to run these tests unless there are ways to run this locally.

@noklam I just added some tests to cover the logic specific to ExternalTableDataset (outside of BaseTableDataset). I also made a few other changes to the existing tstssuch as removing some duplicate code and moving all of the fixtures to conftest.py.

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

noklam · 2024-10-14T13:19:34Z

FAILED tests/databricks/test_base_table_dataset.py::TestBaseTableDataset::test_save_overwrite

Is this an expected fail?

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-14T13:32:21Z

FAILED tests/databricks/test_base_table_dataset.py::TestBaseTableDataset::test_save_overwrite

Is this an expected fail?

@noklam It has been my experience that when overwriting data to an external table of a format other than Delta, the location has to be provided. Since I am an external location fixture here which requires an environment variable called DATABRICKS_EXTERNAL_LOCATION that does not exist in the test environment, yes, this is expected to fail.

I should have specified a different format though. I just made a commit with that change.

Can you give me an explanation of how these tests are running? There should be a Spark environment available for them to run at all, right?

noklam · 2024-10-14T13:36:31Z

Since this is an experimental dataset, test will go to here: https://github.com/kedro-org/kedro-plugins/tree/main/kedro-datasets/kedro_datasets_experimental/tests

This mean tests are not going to be run per every commit in CI. If you look at other Spark tests we run them in a local mode. With Databricks it's unclear whether it possible to run it in a local mode, especially with the platform UnityCatalog (which is different from the open source version).

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-14T13:51:01Z

Since this is an experimental dataset, test will go to here: https://github.com/kedro-org/kedro-plugins/tree/main/kedro-datasets/kedro_datasets_experimental/tests

This mean tests are not going to be run per every commit in CI. If you look at other Spark tests we run them in a local mode. With Databricks it's unclear whether it possible to run it in a local mode, especially with the platform UnityCatalog (which is different from the open source version).

Got it. I just moved the tests; I had to copy the conftest.py from as well.

noklam · 2024-10-18T13:30:39Z

tests are now passing and looks good to me.

merelcht

I haven't tried this out, but the code looks good to me!

Can you please add this addition to the release notes along with your name under contributors? Similar to how it looks here: https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/RELEASE.md#major-features-and-improvements-1

merelcht · 2024-10-18T14:38:01Z

kedro-datasets/kedro_datasets_experimental/databricks/external_table_dataset.py

+        names_and_ages@spark:
+          type: databricks.ExternalTableDataset
+          format: parquet
+          table: names_and_ages
+        names_and_ages@pandas:
+          type: databricks.ExternalTableDataset
+          format: parquet
+          table: names_and_ages
+          dataframe_type: pandas


Nit: can you add some spaces between the entries? It's a bit hard to read where one catalog entry starts and ends like this.

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-18T19:06:54Z

@merelcht I've made the requested changes. Do let me know if I've updated the RELEASE document correctly.

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

merelcht · 2024-10-21T11:03:40Z

@merelcht I've made the requested changes. Do let me know if I've updated the RELEASE document correctly.

I've moved it to the section for upcoming release, because 5.1.0 is already out 🙂 Thank again! I'll make sure this gets merged.

merelcht · 2024-10-21T11:06:20Z

Oh actually @MinuraPunchihewa I just realised this dataset wasn't added to https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/pyproject.toml#L176 or https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/pyproject.toml#L285. Can you update the pyproject.toml with the necessary dependencies if any?

ankatiyar · 2024-10-21T11:09:13Z

Oh actually @MinuraPunchihewa I just realised this dataset wasn't added to https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/pyproject.toml#L176 or https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/pyproject.toml#L285. Can you update the pyproject.toml with the necessary dependencies if any?

Also here: https://github.com/kedro-org/kedro-plugins/blob/main/kedro-datasets/docs/source/api/kedro_datasets_experimental.rst :)

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa · 2024-10-21T12:00:56Z

Hey guys,
Apologies, I've missed this. I've made the changes now.
@merelcht @ankatiyar

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

…ricks (kedro-org#885) * added the experimental ExternalTableDataset * fixed lint issues * added the missing location attr to the docstring * removed unused code from the tests for ManagedTableDataset * added tests for ExternalTableDataset * moved all fixtures to conftest.py * updated the format for the test for save_overwrite to Parquet * moved tests to the kedro_datasets_experimental pkg * updated the release doc * added the dataset to the documentation * listed the dependencies for the dataset --------- Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com> Signed-off-by: tdhooghe <thomas_dhooghe@mckinsey.com>

MinuraPunchihewa added 3 commits October 11, 2024 16:46

added the experimental ExternalTableDataset

0e5a4c4

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

fixed lint issues

61f7771

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added the missing location attr to the docstring

46858c3

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/external_table_dataset branch from 809522d to 46858c3 Compare October 11, 2024 11:39

MinuraPunchihewa marked this pull request as ready for review October 11, 2024 12:03

MinuraPunchihewa added 3 commits October 11, 2024 19:46

removed unused code from the tests for ManagedTableDataset

e9d097e

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

added tests for ExternalTableDataset

b896464

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

moved all fixtures to conftest.py

8354b00

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/external_table_dataset branch from f860c5f to 8354b00 Compare October 11, 2024 14:16

noklam requested review from astrojuanlu, ankatiyar and noklam October 11, 2024 15:31

fixed lint issues

31aecc8

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/external_table_dataset branch from 789c57d to 31aecc8 Compare October 11, 2024 15:46

updated the format for the test for save_overwrite to Parquet

1696d23

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

moved tests to the kedro_datasets_experimental pkg

761f963

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/external_table_dataset branch from aa8c9e1 to 761f963 Compare October 14, 2024 13:50

noklam approved these changes Oct 18, 2024

View reviewed changes

merelcht reviewed Oct 18, 2024

View reviewed changes

MinuraPunchihewa added 2 commits October 19, 2024 00:35

improved spacing in the main docstring

25c3e5b

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

updated the release doc

1577f9a

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

MinuraPunchihewa force-pushed the feature/external_table_dataset branch from 4a87478 to 1577f9a Compare October 18, 2024 19:05

Merge branch 'main' into feature/external_table_dataset

ca63d4c

Add contribution to correct release

d8668ea

Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>

MinuraPunchihewa added 2 commits October 21, 2024 17:29

added the dataset to the documentation

5860688

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

listed the dependencies for the dataset

2451d70

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

fixed indentation in the docstring

fe767e2

Signed-off-by: Minura Punchihewa <minurapunchihewa17@gmail.com>

merelcht enabled auto-merge (squash) October 21, 2024 15:08

merelcht approved these changes Oct 21, 2024

View reviewed changes

merelcht merged commit 5158b3e into kedro-org:main Oct 21, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

MinuraPunchihewa commented Oct 11, 2024 •

edited

Loading

MinuraPunchihewa commented Oct 11, 2024

noklam commented Oct 11, 2024

MinuraPunchihewa commented Oct 11, 2024 •

edited

Loading

noklam commented Oct 14, 2024

MinuraPunchihewa commented Oct 14, 2024

noklam commented Oct 14, 2024

MinuraPunchihewa commented Oct 14, 2024

noklam commented Oct 18, 2024

merelcht left a comment

merelcht Oct 18, 2024

MinuraPunchihewa commented Oct 18, 2024

merelcht commented Oct 21, 2024

merelcht commented Oct 21, 2024

ankatiyar commented Oct 21, 2024

MinuraPunchihewa commented Oct 21, 2024

feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

feat(datasets): Added the Experimental ExternalTableDataset for Databricks #885

Conversation

MinuraPunchihewa commented Oct 11, 2024 • edited Loading

Description

Development notes

Checklist

MinuraPunchihewa commented Oct 11, 2024

noklam commented Oct 11, 2024

MinuraPunchihewa commented Oct 11, 2024 • edited Loading

noklam commented Oct 14, 2024

MinuraPunchihewa commented Oct 14, 2024

noklam commented Oct 14, 2024

MinuraPunchihewa commented Oct 14, 2024

noklam commented Oct 18, 2024

merelcht left a comment

Choose a reason for hiding this comment

merelcht Oct 18, 2024

Choose a reason for hiding this comment

MinuraPunchihewa commented Oct 18, 2024

merelcht commented Oct 21, 2024

merelcht commented Oct 21, 2024

ankatiyar commented Oct 21, 2024

MinuraPunchihewa commented Oct 21, 2024

MinuraPunchihewa commented Oct 11, 2024 •

edited

Loading

MinuraPunchihewa commented Oct 11, 2024 •

edited

Loading