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

Discrepancy in database's run_sql() method #1705

Closed
wants to merge 4 commits into from
Closed

Conversation

rajaths010494
Copy link
Contributor

@rajaths010494 rajaths010494 commented Feb 3, 2023

Please describe the feature you'd like to see
Currently, the handler param in the run_sql() method is only used in data bricks all other databases are not using it, which leads to different response types since the handler is processed within run_sql() for data bricks and not for other databases. Also, the signature of run_sql() in data bricks deviates from the base. We need to standardize and when we do, we can remove the below check as well.

Describe the solution you'd like
Ideally, we should have the same signatures for all the run_sql() database implementations. This will prevent adding explicit conditions for data bricks in other parts of the code that should ideally be database agnostic.
example:

if self.handler and self.database_impl.sql_type == "delta":

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 93.29% // Head: 97.72% // Increases project coverage by +4.43% 🎉

Coverage data is based on head (0a8e65e) compared to base (1aee3ce).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1705      +/-   ##
==========================================
+ Coverage   93.29%   97.72%   +4.43%     
==========================================
  Files          90       21      -69     
  Lines        4726      835    -3891     
  Branches      471        0     -471     
==========================================
- Hits         4409      816    -3593     
+ Misses        227       19     -208     
+ Partials       90        0      -90     
Impacted Files Coverage Δ
python-sdk/src/astro/databases/base.py
python-sdk/src/astro/sql/table.py
...dk/src/astro/sql/operators/export_table_to_file.py
python-sdk/src/astro/sql/operators/dataframe.py
python-sdk/src/astro/databases/postgres.py
python-sdk/src/astro/sql/operators/transform.py
python-sdk/src/astro/files/locations/azure/wasb.py
python-sdk/src/astro/sql/operators/export_file.py
python-sdk/src/astro/sql/__init__.py
python-sdk/src/astro/sql/operators/drop.py
... and 59 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.

@sunank200
Copy link
Contributor

This PR is still a work in progress. This requires a return statement to return the cursor object so that its consistent across all the databases. @rajaths010494 mentioned he would pick this up after other tickets are done for 1.5.1.

@rajaths010494
Copy link
Contributor Author

#1773 PR has been raised to address this issue

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.

None yet

2 participants