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

Move databricks module into the databases directory #1555

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

dimberman
Copy link
Collaborator

Description

What is the current behavior?

Currently the "databricks" module lives at the top level of the python SDK, this can be somewhat confusing as it might lead users to think there is non-database functionality in the databricks module. By moving the databricks module into the database module, we insulate the class and ensure that it is treated like any other database going forward.

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@dimberman dimberman marked this pull request as ready for review January 9, 2023 20:31
@dimberman dimberman linked an issue Jan 9, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Base: 94.00% // Head: 93.21% // Decreases project coverage by -0.79% ⚠️

Coverage data is based on head (143e338) compared to base (116cf74).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
- Coverage   94.00%   93.21%   -0.80%     
==========================================
  Files          89       67      -22     
  Lines        4368     3579     -789     
  Branches      432      432              
==========================================
- Hits         4106     3336     -770     
+ Misses        178      159      -19     
  Partials       84       84              
Impacted Files Coverage Δ
python-sdk/src/astro/databases/__init__.py 100.00% <ø> (ø)
...ricks/load_file/load_file_python_code_generator.py 100.00% <ø> (ø)
...sdk/src/astro/databases/databricks/load_options.py 96.42% <ø> (ø)
...on-sdk/src/astro/databases/databricks/api_utils.py 89.55% <100.00%> (ø)
python-sdk/src/astro/databases/databricks/delta.py 90.14% <100.00%> (ø)
...ro/databases/databricks/load_file/load_file_job.py 93.75% <100.00%> (ø)
sql-cli/sql_cli/operators/load_file.py
sql-cli/sql_cli/configuration.py
sql-cli/sql_cli/astro/utils.py
sql-cli/sql_cli/dag_generator.py
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

Great to see it together with the other databases! I'm happy for it to be merged once the checks are passing.

I wonder if, in a separate ticket, we could try to extend existing fixtures for other DBs and try to unify a bit of tests for load_file and export_file

@kaxil kaxil merged commit 79d69d9 into main Jan 10, 2023
@kaxil kaxil deleted the databricks/move-to-database branch January 10, 2023 12:28
@kaxil
Copy link
Collaborator

kaxil commented Jan 10, 2023

Link: https://deepsource.io/gh/astronomer/astro-sdk/run/28eec7e3-e48b-4c50-ad29-2e6f90298642/python/
image

2 of the 3 suggestions from deep source should be fixed -- but since it wasn't added in this PR, please create a separate PR to fix those


The other test failure was not related to this PR -- but probably flaky that we should check too separately - around GCS to BQ load

@phanikumv
Copy link
Collaborator

Sure @kaxil - we will create a PR for that

cc @pankajastro

@pankajastro
Copy link
Contributor

pankajastro commented Jan 11, 2023

Sure @kaxil - we will create a PR for that

cc @pankajastro

@phanikumv @kaxil I tried to make changes in these files to re-produce it but I'm unable to re-reproduce #1565 https://deepsource.io/gh/astronomer/astro-sdk/run/e3fb7ef7-1f58-4f80-bcf8-96d53b2012d2/python/ but I have improved some check as suggested by deepsource

@pankajastro pankajastro mentioned this pull request Jan 11, 2023
2 tasks
pankajastro added a commit that referenced this pull request Jan 12, 2023
# Description
Address:
#1555 (comment)
## What is the current behavior?
Fix some deepsource issues


## Does this introduce a breaking change?
No

### Checklist
- [ ] Created tests which fail without the change (if possible)
- [ ] Extended the README / documentation, if necessary
utkarsharma2 pushed a commit that referenced this pull request Jan 17, 2023
## What is the current behavior?
Currently the "databricks" module lives at the top level of the python
SDK, this can be somewhat confusing as it might lead users to think
there is non-database functionality in the databricks module. By moving
the databricks module into the database module, we insulate the class
and ensure that it is treated like any other database going forward.
utkarsharma2 pushed a commit that referenced this pull request Jan 17, 2023
# Description
Address:
#1555 (comment)
## What is the current behavior?
Fix some deepsource issues


## Does this introduce a breaking change?
No

### Checklist
- [ ] Created tests which fail without the change (if possible)
- [ ] Extended the README / documentation, if necessary
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.

Move databricks from top-level module to astro/databases
5 participants