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

docs: explain you only need to close a conn.raw_sql() if you are running a query that returns results #8345

Closed
1 task done
NickCrews opened this issue Feb 14, 2024 · 2 comments · Fixed by #8417
Closed
1 task done
Labels
docs Documentation related issues or PRs

Comments

@NickCrews
Copy link
Contributor

Please describe the issue

I was following the advise of https://ibis-project.org/how-to/extending/sql#backend.raw_sql, and closing the returned cursor.

But the actual SQL I was running was an EXPLAIN SELECT ... query. I was getting errors, eventually after a lot of headscratching I think I have found that I do NOT want to close the cursor after this query.

Possible actions:

  1. be more precise with what sorts of SQL statements require the close.
  2. make it so ALL queries require a close, so there is no distinction.
  3. UX: make the returned object itself closable, so I don't have to do from contextlib import closing and I can just do with conn.raw_sql(x) as cursor:

Code of Conduct

  • I agree to follow this project's Code of Conduct
@NickCrews NickCrews added the docs Documentation related issues or PRs label Feb 14, 2024
NickCrews added a commit to NickCrews/mismo that referenced this issue Feb 14, 2024
There was a big refactor in ibis to use sqlglot.
This uses the new changes and APIs,
and fixes the flaky tests from the closed cursor due to
ibis-project/ibis#8345
@cpcloud
Copy link
Member

cpcloud commented Feb 22, 2024

@NickCrews Thanks for the issue!

I think option 1 is probably the best we can do here without a ton of additional effort.

make it so ALL queries require a close, so there is no distinction.

This is out of the control of Ibis. There's no agreed-upon-by-all-backends criteria for whether a cursor needs to be closed. The documentation is best effort. I can certainly update it to reflect the variation.

make the returned object itself closable

We're unlikely to do this because it would require wrapping the backend's cursor, and the point of raw_sql is to expose the backend's cursor directly, without interfering. The trade off is that users must now deal with any inconveniences brought on by the backend's driver.

@cpcloud
Copy link
Member

cpcloud commented Feb 22, 2024

To clarify, you should only need to close the cursor if you're doing a SELECT statement, or more generally a statement that can return values. DDL statements should not require closing the cursor.

@cpcloud cpcloud changed the title docs: explain you only need to close a conn.raw_sql() if you are doing a DDL statement docs: explain you only need to close a conn.raw_sql() if you are running a query that returns results Feb 22, 2024
gforsyth added a commit that referenced this issue Feb 22, 2024
Closes #8345.

---------

Co-authored-by: Gil Forsyth <gforsyth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants