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 #1664

Closed
1 task
utkarsharma2 opened this issue Jan 23, 2023 · 3 comments · Fixed by #1773
Closed
1 task

Discrepancy in database's run_sql() method #1664

utkarsharma2 opened this issue Jan 23, 2023 · 3 comments · Fixed by #1773
Assignees
Labels
feature New feature or request priority/low Low priority
Milestone

Comments

@utkarsharma2
Copy link
Collaborator

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":

Acceptance Criteria

  • Where ever we have used run_sql(), we shouldn't be having any database-specific checks.
@sunank200
Copy link
Contributor

sunank200 commented Feb 10, 2023

This PR is still a work in progress. This requires a return statement to return the cursor object to the generated result so that its consistent across all the databases. @rajaths010494 mentioned he would pick this up after other tickets are done for 1.5.1. He needs to sync with @dimberman for this as well @rajaths010494 to provide more details

@sunank200
Copy link
Contributor

@rajaths010494 has set up the meeting today. He will provide more details on this ticket.

@rajaths010494
Copy link
Contributor

rajaths010494 commented Feb 15, 2023

Connected with @dimberman regarding run_sql we would be using get_wrapped_handler to get results for other databases and make necessary other changes for tests

rajaths010494 added a commit that referenced this issue Feb 20, 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:
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
utkarsharma2 pushed a commit that referenced this issue Feb 20, 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:
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
utkarsharma2 pushed a commit that referenced this issue Feb 21, 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:
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request priority/low Low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants