Skip to content

Magic params flag fails with negative numbers #108

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

Closed
teebr opened this issue May 13, 2020 · 3 comments · Fixed by #119 or #213
Closed

Magic params flag fails with negative numbers #108

teebr opened this issue May 13, 2020 · 3 comments · Fixed by #119 or #213
Assignees
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. type: docs Improvement to the documentation for an API.

Comments

@teebr
Copy link

teebr commented May 13, 2020

Environment details

Colab (Ubuntu 18.04 LTS, Python 3.6.9, pip 19.3.1, bigquery 1.21.0)

Issue/ Example

For a parameterised query, the magic fails to parse negative numbers.

For example, this parameter dict:

params = {
    "threshold": -5.0,
    "min_year": 2000
}

and this query:

%%bigquery  --project my-project-name --params $params
SELECT
    year, MIN(mean_temp) as min, COUNTIF(snow) / COUNT(snow) * 100 AS pc_snowing
FROM
     `bigquery-public-data.samples.gsod`
WHERE mean_temp < @threshold
AND year >= @min_year
GROUP BY year
ORDER BY year

runs fine if threshold is >= 0, but if it's negative, the Magic gets an exception:

UsageError: unrecognized arguments: -5.0, 'min_year': 2000}

This happens whether it's a dictionary or a JSON string, and happens with ints as well as floats.

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label May 13, 2020
@teebr
Copy link
Author

teebr commented May 13, 2020

It seems this happens in the IPython's magic_arguments call to arg_split. That uses shlex, and I think that interprets whitespace followed by a hyphen as a new argument. Anyway, dumping the dictionary to JSON with no whitespace does work:

pj = json.dumps(params, separators=(',', ':'))
...
%%bigquery  --project my-project-name --params $pj

@plamut plamut added the type: question Request for information or clarification. Not an issue. label May 14, 2020
@plamut
Copy link
Contributor

plamut commented May 14, 2020

@teebr Interesting, thanks for the report!

I was able to reproduce it, as well as confirming that the workaround works. We now need to see if there's an easy fix to that.

@plamut plamut added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels May 14, 2020
@plamut plamut self-assigned this Jun 2, 2020
@plamut
Copy link
Contributor

plamut commented Jun 2, 2020

This doesn't seem easily solvable, unfortunately. Using the following minimal example, for instance:

>>> params = {"num": -17}
%%bigquery --params $params
SELECT @num as num

passes the following line contents to the BigQuery's _cell_magic() function:

>>> line
"--params {'num': -17}"

which is tokenized in IPython's MagicArgumentParser.parse_argstring() method as follows:

>>> argv
['--params', '{"num":', '-17}']

The tokenizer can't really know that it should treat the tokens after "--params" as a single entity representing a Python dict, thus it's understandable why the above fails - -17} is treated as a yet another argument, and not as part of the value of another argument.

BigQuery's magic actually has to manually reconstruct the dict from these fragments, which is a bit of a hack that deals with the symptom:

ast.literal_eval("".join(args.params))

In order to force the parser to treat the params dict as a single entity, the dict should be passed in as a JSON string as @teebr noted, and this limitation should be documented in bigquery cell magic docs.

Even if not perfect, this solution probably has the best benefit/cost ratio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. type: docs Improvement to the documentation for an API.
Projects
None yet
2 participants