-
Notifications
You must be signed in to change notification settings - Fork 324
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
feat: add sqlglot to parse sql dataflow #3310
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic, thank you so much! I'm very excited by what a migration to sqlglot will unlock in the future, including support for other dialects.
Left one nit and one question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment, and then looks good to me. Thanks for this change!
tests/_ast/test_sql_visitor.py
Outdated
@staticmethod | ||
def test_find_sql_refs_with_recursive_cte() -> None: | ||
sql = """ | ||
WITH RECURSIVE cte AS ( | ||
SELECT 1 AS n FROM table1 | ||
UNION ALL | ||
SELECT n + 1 FROM cte WHERE n < 10 | ||
) | ||
SELECT * FROM cte; | ||
""" | ||
assert find_sql_refs(sql) == ["table1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not yet supported, but you'd like to keep the test, you can decorate it with pytest.mark.xfail
, as done here:
marimo/tests/_ast/test_sql_visitor.py
Line 569 in f8dc9f5
@pytest.mark.xfail(reason="Multiple CTEs are not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does work, I didn't see the need for it since we have tested nested cte's. Also noted on the dialect change. I may add back the has_duckdb dependency too.
thanks!!
def test_find_sql_refs_invalid_sql() -> None: | ||
sql = "SELECT * FROM" | ||
assert find_sql_refs(sql) == [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! This will have a large impact on marimo's SQL support; thanks so much for getting the ball rolling!
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.10.10-dev1 |
📝 Summary
Fixes #3103. Adds sqlglot as an optional dependency to handle complex sql refs parsing. Also supports DML statements (insert, update and delete).
🔍 Description of Changes
def find_sql_def()
can also be modified to use sqlglot in the future, not included in this PR.📋 Checklist
📜 Reviewers
@akshayka OR @mscolnick