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

Fixing argument parsing on Windows #425

Closed
edublancas opened this issue Apr 21, 2023 · 2 comments · Fixed by #822
Closed

Fixing argument parsing on Windows #425

edublancas opened this issue Apr 21, 2023 · 2 comments · Fixed by #822
Assignees

Comments

@edublancas
Copy link

When working on #424, I realized we have an error when parsing the arguments passed to the SQL query (e.g., %sql SELECT * FROM table --persist - note that we have to parse the query and the --persist argument).

This is because we're relying on parse_argstring from IPython, which behaves differently on Windows and will delete quotes that are necessary for a SQL query to run, here's how to reproduce it.

from IPython.core.magic_arguments import parse_argstring
from sql.magic import SqlMagic
from IPython import get_ipython
%load_ext sql

Console output (1/1):

The sql extension is already loaded. To reload it, use:
  %reload_ext sql
ip = get_ipython()
magic = ip.find_magic("sql")

We're using this function internally to parse arguments, note that the query has "table spaces" in double quote:

parse_argstring(magic, 'create table "table spaces" (n INT)').line

On Windows, the double quotes disappear (this doesn't happen on mac or linux):

Console output (1/1):

['create', 'table', 'table spaces', '(n', 'INT)']

This happens because on linux/mac, the _process_common function is used:

from IPython.utils._process_common import arg_split
arg_split('create table "table spaces" (n INT)')

Console output (1/1):

['create', 'table', '"table spaces"', '(n', 'INT)']

But on Windows, the _process_win32 is used:

from IPython.utils._process_win32 import arg_split
arg_split('create table "table spaces" (n INT)')

Console output (1/1):

['create', 'table', 'table spaces', '(n', 'INT)']

I think we can fix this by changing our magic_args function in parse.py:

import shlex

def magic_args(magic_execute, line):
    line = without_sql_comment(parser=magic_execute.parser, line=line)
    return magic_execute.parser.parse_args(shlex.split(line, posix=False))

But I'm unsure if there would be any side effects.


Some IPython links:

https://github.com/ipython/ipython/blob/a3d5a06d9948260a6b08c622e86248f48afd66c3/IPython/utils/_process_win32.py#L29

https://github.com/ipython/ipython/blob/a3d5a06d9948260a6b08c622e86248f48afd66c3/IPython/utils/_process_common.py#L25

https://github.com/ipython/ipython/blob/a3d5a06d9948260a6b08c622e86248f48afd66c3/IPython/core/magic_arguments.py#L164

@bbeat2782
Copy link

Acceptance Criteria

  1. Using either double or single quote to enclose a space when using %sql, %sqlplot, or %sqlcmd should work. (e.g %sql create table "table spaces" (n INT), %sqlplot bar --table 'file with spaces.csv' --column x, or %sqlcmd columns -t 'table with spaces' should work)
  2. Add test functions

@edublancas
Copy link
Author

go ahead! the easiest way to get started is to try the solution I propose at the end, and then see if all tests pass and if there are any edge cases

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 a pull request may close this issue.

2 participants