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

fix result when summarize is passed to duckdb #841

Merged
merged 15 commits into from
Sep 1, 2023
Merged

fix result when summarize is passed to duckdb #841

merged 15 commits into from
Sep 1, 2023

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Aug 28, 2023

Describe your changes

Now the correct result is displayed when SUMMARIZE is passed to duckdb with sqlalchemy connection

testing_summarize.ipynb.zip

Issue number

Closes #836

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--841.org.readthedocs.build/en/841/

@bbeat2782
Copy link
Author

For context,

With sqlalchemy==1.4.49, result of query is stored in LegacyCursorResult. Not sure why, but result stored here remains even after commit().
In sqlalchemy==2.x.x, LegacyCursorResult is removed, and result is stored in CursorResult. In this case, the CursorResult is emptied after commit() : related issue

So as @edublancas suggested here, returning the CursorResult without commit() (like we do with select, with, and from for duckdb) solves the problem.

@bbeat2782 bbeat2782 marked this pull request as ready for review August 28, 2023 20:18
@bbeat2782 bbeat2782 requested a review from edublancas as a code owner August 28, 2023 20:18
@@ -727,7 +727,7 @@ def _connection_execute(self, query, parameters=None):
# TODO: we can parse the query to ensure that it's a SELECT statement
# for example, it might start with WITH but the final statement might
# not be a SELECT
is_select = first_word_statement in {"select", "with", "from"}
is_select = first_word_statement in {"select", "with", "from", "summarize"}

Choose a reason for hiding this comment

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

please add a note explaining why we're adding "summarize" here

Copy link
Author

Choose a reason for hiding this comment

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

@bbeat2782 bbeat2782 requested a review from edublancas September 1, 2023 19:44
@edublancas edublancas merged commit f5c2137 into ploomber:master Sep 1, 2023
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.

[duckdb] SUMMARIZE tbl; doesn't return anything
2 participants