feat(datasets): Refactored the ManagedTableDataset by Separating Out Common Table Logic

[MinuraPunchihewa]

Description

This PR refactors the ManagedTableDataset by separating our the common 'table' (managed and external) logic to a separate BaseTableDataset.

Development notes

The common table logic that is used to execute operations on Databricks Unity Catalog has been separated out here, with the primary purpose to be the implementation of an ExternalTableDataset in the near future.

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

[MinuraPunchihewa]

@noklam As requested, I have updated this PR to only contain the changes related to the refactor. From the CI runs, it is not entirely clear to me why the lint and unit tests are failing though. Is there anything else that I need to do here?

[noklam]

Generally looks fine for me. If you have to add new argument, make sure it's a non-breaking one, i.e. add it in the end instead of changing the existing order.

[noklam]

Can the order be preserved? this would be a breaking change.

[MinuraPunchihewa]

I just pushed a commit. Does this resolve your concern? Do the order of operations matter here since I am passing in keyword arguments?

[noklam]

The dataset class order doesn't matter because it is already a keyword only class.

The manage table class however is technically still a public class so change of order is a breaking change. It's not gonna affect too much since it's rare but I prefer only introduce breaking change when necessary.

[MinuraPunchihewa]

@noklam Very clear. Thank you.

[ankatiyar]

Hey @MinuraPunchihewa, the refactoring looks good to me but the test coverage for kedro_datasets/databricks/_base_table_dataset.py is not complete which is causing the CI to fail. You might also need to run make plugin=kedro-datasets lint and push the linter changes.

[MinuraPunchihewa]

@ankatiyar @noklam I am working on the tests now. Can you please let me know how I can run them?
I tried make plugin=kedro-datasets test and pytest tests/databricks/test_managed_table_dataset.py, but I get the following error:

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov-report --cov-report term-missing --cov kedro_datasets --cov tests --no-cov-on-fail tests --cov-config pyproject.toml --numprocesses 4 --dist loadfile
  inifile: /home/minura/Documents/work/kedro/repos/kedro-plugins/kedro-datasets/pyproject.toml
  rootdir: /home/minura/Documents/work/kedro/repos/kedro-plugins/kedro-datasets

Is it possible to run only a particular test module with make?

[noklam]

Have you run make plugin=kedro-datasets install-test-requirements already? You are missing the testing dependencies (the error is complainign pytest-cov)

[MinuraPunchihewa]

@noklam Thank you; I was able to get it running after installing the dependencies.
@ankatiyar added some tests; what I basically did was take the existing tests for managed_table_dataset.py and moved it to _base_table_dataset.py. Then I added a simple tests for the describe() method of managed_table_dataset.py because that is the only place where it differs. Let me know what you think.
I've fixed the lint issues as well.

[MinuraPunchihewa]

Hey @noklam, @ankatiyar,
I needed to add a couple of other tests to get the coverage to 100%, but a couple of these tests require an external location to save the external tables in. I've added this in as a fixture which takes the location from an environment variable called DATABRICKS_EXTERNAL_LOCATION. Is it possible to set this up in Databricks environment configured for testing or is there a better way to go about this?

[ankatiyar]

Thanks @MinuraPunchihewa!

[ankatiyar]

I dont believe we have a test databricks setup but we can look at how to deal with the tests when you're ExternalTableDataset PR is ready. I'll try to get this refactor in for now.

[MinuraPunchihewa]

I dont believe we have a test databricks setup but we can look at how to deal with the tests when you're ExternalTableDataset PR is ready. I'll try to get this refactor in for now.

Thank you, @ankatiyar. Super stoked to have this merged.
Cool. May I know how the unit tests are running though? The unit tests that existed for ManagedTableDataset seemed to require a Spark environment to run from what I could gather.