Use handler in run_sql #1773
Use handler in run_sql #1773
Conversation
Codecov ReportBase: 97.72% // Head: 97.72% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1773 +/- ##
=======================================
Coverage 97.72% 97.72%
=======================================
Files 21 21
Lines 835 835
=======================================
Hits 816 816
Misses 19 19 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. |
There was a problem hiding this comment.
Nice to see an alignment of the interface cross-databases, @rajaths010494 ! I'm happy for this change to be merged once the checks are passing.
Since this is a breaking change, we just need to remember to add this to the release notes. WDYT if we already added to the changelog?
The changelog PR is not yet created will make sure to add this change to that. |
rajaths010494
requested review from
dimberman,
utkarsharma2,
sunank200,
pankajastro,
pankajkoti and
feluelle
as code owners
**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: https://github.com/astronomer/astro-sdk/blob/2f8c8d0ccbff3cc13af78685cbefa39f473991c3/python-sdk/src/astro/sql/operators/raw_sql.py#L47 **Acceptance Criteria** - [ ] Where ever we have used run_sql(), we shouldn't be having any database-specific checks. closes #1664
**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: https://github.com/astronomer/astro-sdk/blob/2f8c8d0ccbff3cc13af78685cbefa39f473991c3/python-sdk/src/astro/sql/operators/raw_sql.py#L47 **Acceptance Criteria** - [ ] Where ever we have used run_sql(), we shouldn't be having any database-specific checks. closes #1664
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 ofrun_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:
astro-sdk/python-sdk/src/astro/sql/operators/raw_sql.py
Line 47 in 2f8c8d0
Acceptance Criteria
closes #1664