-
Notifications
You must be signed in to change notification settings - Fork 76
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
658 snippet support sqlcmd #721
658 snippet support sqlcmd #721
Conversation
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.
minor comments. I tested it and it works well
src/sql/inspect.py
Outdated
@@ -173,7 +174,7 @@ class Columns(DatabaseInspection): | |||
""" | |||
|
|||
def __init__(self, name, schema, conn=None) -> None: | |||
util.is_table_exists(name, schema) | |||
util._check_table_exists(name, schema) |
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.
so I see you added logic here that will let this continue if name is defined as a snippet. I think that makes sense so this feature works but I wonder if it'll have any side-effects since we're using this class in multiple places
@yafimvo, thoughts?
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.
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.
Hi, the _check_table_exists
function first checks if the table is a snippet name, and if it is not we run the is_table_exists
inside the earlier function. So, if we pass a table name that is neither a snippet or a table, this will work similar to the check we had before.
@AnirudhVIyer please fix merge conflicts in the changelog |
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.
@AnirudhVIyer
Added some comments
src/sql/inspect.py
Outdated
@@ -173,7 +174,7 @@ class Columns(DatabaseInspection): | |||
""" | |||
|
|||
def __init__(self, name, schema, conn=None) -> None: | |||
util.is_table_exists(name, schema) | |||
util._check_table_exists(name, schema) |
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.
…dhVIyer/jupysql into 658-snippet-support-sqlcmd merge origin
src/sql/util.py
Outdated
name of the snippet | ||
""" | ||
with_ = None | ||
if is_saved_snippet(table, task): |
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.
I think task
shouldn't be a parameter of is_saved_snippet
. Can refactor this a bit:
if is_saved_snippet(table):
print(f"{msg} using saved snippet : {table}")
And delete the print statement inside is_saved_snippet
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.
Made this change, now we send a message in is_saved_snippet_or_table_exists
itself.
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.
@neelasha23 why do we need a message at all? The users already know the action they are performing (since they typed:%sqlcmd explore
or %sqlcmd profile
) and they know it's a snippet (if not, does it matter?).
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.
The message was added as part of the CTE deprecation issue. initially it was only plotting
.. it was needed to clarify that we are plotting using saved snippet. @yafimvo
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 it's clear anyways then I think we can remove it
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.
Sure! removing it
The errors are fixed. |
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.
looks good!
@AnirudhVIyer this PR has lots of conflicts now because we merge a PR that refactored a lot of things. check how difficult it is to fix the merge conflicts, alternatively, you can create a new PR and manually apply your changes |
@edublancas fixed the Merge conflicts |
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.
please fix tests. I think a lot of the will fail because the .execute and ._prepare_query methods are a bit inconsistent (I'll work on a PR soon)
in the meantime, please address the observations in the tests, if some of the databases fail. it's fine, you can add a pytest.param(..., mark=pytest.mark.xfail(..))
(see existing tests to see how) this will allow the test to fail.
as long as duckdb sqlite and postgres pass, we can check the other databases one by one and see what's going on
ip_with_dynamic_db = request.getfixturevalue(ip_with_dynamic_db) | ||
ip_with_dynamic_db.run_cell( | ||
""" | ||
%%sql sqlite:// |
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 you pass sqlite:// you'll overwrite the db and you'll be testing with sqlite instead of the database from the ip_with_dynamic_db, please remove
got it! making the changes |
cool, don't forget to request a review when ready |
if ip_with_dynamic_db not in [ | ||
"ip_with_postgreSQL", | ||
"ip_with_duckDB", | ||
"ip_with_SQLite", | ||
]: | ||
pytest.xfail(reason="There may be an issue due to code refactoring") |
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.
let the tests run and only exclude the ones where we are sure they'll fail
%%sql | ||
CREATE TABLE people (name varchar(50), number int, country varchar(50)); | ||
INSERT INTO people VALUES ('joe', 82, 'usa'); | ||
INSERT INTO people VALUES ('paula', 93, 'uk'); | ||
INSERT INTO people VALUES ('sam', 77, 'canada'); | ||
INSERT INTO people VALUES ('emily', 65, 'usa'); | ||
INSERT INTO people VALUES ('michael', 89, 'usa'); | ||
INSERT INTO people VALUES ('sarah', 81, 'uk'); | ||
INSERT INTO people VALUES ('james', 76, 'usa'); | ||
INSERT INTO people VALUES ('angela', 88, 'usa'); | ||
INSERT INTO people VALUES ('robert', 82, 'usa'); | ||
INSERT INTO people VALUES ('lisa', 92, 'uk'); | ||
INSERT INTO people VALUES ('mark', 77, 'usa'); | ||
INSERT INTO people VALUES ('jennifer', 68, 'australia'); | ||
""" | ||
) | ||
ip_with_dynamic_db.run_cell( | ||
""" | ||
%%sql --save citizen | ||
select * from people where country = 'usa' | ||
""" | ||
) |
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.
we're already creating some tables for all databases (although they have different names), check out how we're getting the table names. there's no need to create new tables here
select * from people where country = 'usa' | ||
""" | ||
) | ||
with capture_output() as captured: |
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.
we're using pytests capsys for this. it works pretty much the same, let's replace it so our tests are consistent
%%sql | ||
CREATE TABLE people (name varchar(50), number int, country varchar(50)); | ||
INSERT INTO people VALUES ('joe', 82, 'usa'); | ||
INSERT INTO people VALUES ('paula', 93, 'uk'); | ||
INSERT INTO people VALUES ('sam', 77, 'canada'); | ||
INSERT INTO people VALUES ('emily', 65, 'usa'); | ||
INSERT INTO people VALUES ('michael', 89, 'usa'); | ||
INSERT INTO people VALUES ('sarah', 81, 'uk'); | ||
INSERT INTO people VALUES ('james', 76, 'usa'); | ||
INSERT INTO people VALUES ('angela', 88, 'usa'); | ||
INSERT INTO people VALUES ('robert', 82, 'usa'); | ||
INSERT INTO people VALUES ('lisa', 92, 'uk'); | ||
INSERT INTO people VALUES ('mark', 77, 'usa'); | ||
INSERT INTO people VALUES ('jennifer', 68, 'australia'); | ||
""" | ||
) |
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.
use existing tables
for row in stats_table: | ||
profile_metric = _get_row_string(row, " ") | ||
name = _get_row_string(row, "name") | ||
number = _get_row_string(row, "number") | ||
country = _get_row_string(row, "country") | ||
|
||
assert profile_metric in expected | ||
assert name == str(expected[profile_metric][0]) | ||
assert number == str(expected[profile_metric][1]) | ||
assert country == str(expected[profile_metric][2]) |
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.
keep tests simples, using for
or if
makes them harder to understand. try simplifying this while removing this for loop
%%sql | ||
CREATE TABLE people (name varchar(50), number int, country varchar(50)); | ||
INSERT INTO people VALUES ('joe', 82, 'usa'); | ||
INSERT INTO people VALUES ('paula', 93, 'uk'); | ||
INSERT INTO people VALUES ('sam', 77, 'canada'); | ||
INSERT INTO people VALUES ('emily', 65, 'usa'); | ||
INSERT INTO people VALUES ('michael', 89, 'usa'); | ||
INSERT INTO people VALUES ('sarah', 81, 'uk'); | ||
INSERT INTO people VALUES ('james', 76, 'usa'); | ||
INSERT INTO people VALUES ('angela', 88, 'usa'); | ||
INSERT INTO people VALUES ('robert', 82, 'usa'); | ||
INSERT INTO people VALUES ('lisa', 92, 'uk'); | ||
INSERT INTO people VALUES ('mark', 77, 'usa'); | ||
INSERT INTO people VALUES ('jennifer', 68, 'australia'); | ||
""" | ||
) |
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.
use existing tables
Describe your changes
Added suport for snippets in
explore
andprofile
commands.In each command now we check if the table name is a reference to a table or a snippet
Use an internal variable with_clause that changes the SQL query enabling support for snippets in these commands.
Issue number
Closes #658
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--721.org.readthedocs.build/en/721/