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 duckdb leading comments #895

Merged
merged 26 commits into from
Oct 11, 2023
Merged

Conversation

marshallwhiteorg
Copy link

@marshallwhiteorg marshallwhiteorg commented Sep 22, 2023

Describe your changes

Fix empty result in certain duckdb SELECT and SUMMARIZE queries with leading comments

Issue number

Closes #892

Checklist before requesting a review


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

@edublancas
Copy link

@marshallwhiteorg any updates here?

@marshallwhiteorg
Copy link
Author

@edublancas
Copy link

edublancas commented Sep 27, 2023

some of these look like matplotlib issues. I think it got upgraded recently and we had to pin the version:

"matplotlib==3.7.2",

merging from master might help

@marshallwhiteorg
Copy link
Author

@tonykploomber @neelasha23

I have all the tests passing except some of the docs are failing to build. My code is using sqlglot.parse_one to parse the duckdb queries and that function doesn't accept this type of query:

%%sql duckdb:///
INSTALL 'sqlite_scanner';
LOAD 'sqlite_scanner';

The error is in here: https://readthedocs.org/projects/jupysql/builds/22054160/

Here's an example.

In [44]: from sqlglot import parse_one
In [45]: parse_one("INSTALL 'sqlite_scanner'", dialect="duckdb")
...
ParseError: Invalid expression / Unexpected token. Line 1, Col: 24.
  INSTALL 'sqlite_scanner'

Can you see what I am doing wrong, or do you know why this used to work? Is there some preprocessing step that sqlglot requires for this query? It looks like valid duckdb syntax based on their docs. Also, it works with double quotes but not single quotes. Thanks!

@neelasha23
Copy link

I can see this query is failing:

%%sql
SELECT 
    name, 
    friends[1] AS first_friend, 
    likes.pizza AS likes_pizza, 
    likes.tacos AS likes_tacos
FROM read_json_auto('people.json')

The issue might be in this line : friends[1] AS first_friend,

It looks like sqlglot had a release yesterday : https://pypi.org/project/sqlglot/#history
You can check the release notes to see if there's any breaking changes. Or you can pin sqlglot version < 18.8.0 and rerun the CI. If it fixes the problem open an issue that the latest sqlglot is causing queries to break. @marshallwhiteorg

…nt. This may happen in the case of a statement like INSTALL 'xxx'. In these cases we assume the statement is not select or summarize.
@marshallwhiteorg marshallwhiteorg marked this pull request as ready for review October 3, 2023 05:39
src/sql/connection/connection.py Show resolved Hide resolved
src/sql/connection/connection.py Outdated Show resolved Hide resolved
src/tests/test_magic.py Outdated Show resolved Hide resolved
src/tests/test_magic.py Outdated Show resolved Hide resolved
@marshallwhiteorg
Copy link
Author

@edublancas @idomic @neelasha23 Okay I've made the requested changes, please let me know if you see issues with the new version.

I was skeptical that sqlparse would be up to the task but I was only able to make it fail once where sqlglot succeeded.
For the query SELECT FROM table WHERE (column = 'value' sqlglot correctly marks it as invalid due to the missing close paren but sqlparse does not. I marked the test as xfail. Let me know if you have any more test cases you'd like me to add.

Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

minor stuff. remember to respond to PR comments with a link to your changes: https://ploomber-contributing.readthedocs.io/en/latest/contributing/responding-pr-review.html

src/sql/connection/connection.py Outdated Show resolved Hide resolved
src/sql/connection/connection.py Show resolved Hide resolved
src/tests/test_connection.py Outdated Show resolved Hide resolved
src/tests/test_connection.py Outdated Show resolved Hide resolved
@edublancas
Copy link

@marshallwhiteorg addressed the comments. please check the open conversations and request my review when done

@marshallwhiteorg
Copy link
Author

@edublancas Thanks for the comments, I just responded to both and made the assert -> NotImplementedError change, see my comment. Ready for review again

edublancas
edublancas previously approved these changes Oct 9, 2023
Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

lgtm. @neelasha23 please run some manual tests and let us know if everything looks good

neelasha23
neelasha23 previously approved these changes Oct 11, 2023
Copy link

@neelasha23 neelasha23 left a comment

Choose a reason for hiding this comment

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

lgtm!

@neelasha23
Copy link

please resolve merge conflicts @marshallwhiteorg

@edublancas
Copy link

@marshallwhiteorg please resolve merge conflicts and let me know so we can merge this

@marshallwhiteorg
Copy link
Author

@edublancas @neelasha23 Fixed merge conflicts

@edublancas
Copy link

@marshallwhiteorg tests are failing

@marshallwhiteorg
Copy link
Author

@edublancas fixed

@edublancas edublancas merged commit 3b0b1c0 into ploomber:master Oct 11, 2023
23 checks passed
@marshallwhiteorg marshallwhiteorg deleted the sql-comments branch October 11, 2023 19:37
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.

adding SQL comments breaks jupysql
3 participants