Variable expansion outside queries

[neelasha23]

Describe your changes

Added variable expansion support for the string type arguments in the following:

1. %sqlcmd columns
2. %sqlcmd explore
3. %sqlcmd profile
4. %sqlcmd snippets
5. %sqlcmd tables
6. %sqlcmd test
7. %sql
8. %sqlplot

Integer/ float arguments of histogram plot like bins, binwidth, etc are not supported.

Issue number

Closes #699

Checklist before requesting a review

• Performed a self-review of my code
• Formatted my code with pkgmt format
• Added tests (when necessary).
• Added docstring documentation and update the changelog (when needed)

---

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

[neelasha23]

I haven't added histogram specific arguments like bins, binwidth etc, and comparator arguments for testing like greater, greater_or_equal etc. Do we need to add these as well? @edublancas

[edublancas]

@neelasha23 I think expanding the parameters when we read the arguments string would be better so we automatically add expansion to them. Otherwise, we'll have to remember to add this feature to every argument we add. If you look at the SQL magic implementation, there should be a part where we obtain the raw argument string e.g. --a something --b another; we could expand the values there

[neelasha23]

Addressed the comment. Please review @edublancas

[edublancas]

@bryannho please review this, pay special attention to this comment

[bryannho]

Overall the variable expansion works well, but I have some questions. Of course the intention is to use variables to specify argument values like this:

%%sql --save {{foo}} --no-execute
SELECT 1

But in testing I found that you can also use them to specify the argument flags themselves, like this:

arg = "--save"

%%sql {{arg}} snippet --no-execute
SELECT 1

This works fine when the arguments are specified correctly. However, I could do something like

arg = "--not_an_arg"

and the statement will still run without throwing an error:

@edublancas @neelasha23 Do we want to allow users to specify argument flags using variables in this way? I assume this wasn't the original intent. It could definitely be helpful but might also open up more issues in the future.

If so, I think we will need to call expand_args() before we parse/check for invalid arguments. We will also need to include this in the documentation and spend more time thinking about potential corner cases/bugs.

[neelasha23]

@edublancas @neelasha23 Do we want to allow users to specify argument flags using variables in this way? I assume this wasn't the original intent. It could definitely be helpful but might also open up more issues in the future.

I think for this PR we should only handle arguments and not flags. I have changed the approach. Previously I had tried a couple of approaches:

1. Regex based to extract --arg {{value}} patterns and then rendering. This works but I'm not very confident of this approach since false edge cases/ missed cases might come up.
2. Allow the argparse parser to parse the input as is and then substitute the args. So the argparse.Namespace would contain values like save: {{snippet}} and then {{snippet}} would be rendered by the actual snippet name. I feel this approach is less error prone and more deterministic, as now we are rendering only the argument values specifically, although it requires an additional step of iterating over the namespace. So I have reverted to this approach.

Here are the changes:

columns
explore
profile
snippets
tables
test
command
magic
magic_plot
util functions
util functions

Regarding the error, now it will display error if we try to use the snippet:

@edublancas @bryannho

[bryannho]

@neelasha23 Manual testing checks out. Overall your last solution was more streamlined but I can see why you needed to revert back to this approach in order to disallow parametrizing flags. I'll leave it up to @edublancas if he feels like this is too clunky. If we were to add a new command it would have to remember to include this logic:

 args = parser.parse_args(others)

    if is_rendering_required(" ".join(others)):
        expand_args(args, user_ns)

Not sure if we even plan on adding new commands anytime soon though.

It would be helpful to add a warning to users when they try to parametrize a flag. Even though the flag isn't registered, currently the command still executes so users may be confused why it isn't working. So if the user tried this:

arg = "--save"

%%sql {{arg}} snippet
SELECT 1

They should receive UserWarning: Parametrization of argument flags is not allowed or something like that.

[edublancas]

Allow the argparse parser to parse the input as is and then substitute the args. So the argparse.Namespace would contain values like save: {{snippet}} and then {{snippet}} would be rendered by the actual snippet name. I feel this approach is less error prone and more deterministic, as now we are rendering only the argument values specifically, although it requires an additional step of iterating over the namespace. So I have reverted to this approach.

I'm confused. this approach sounds like a much more robust alternative than adding code to each of the SQL commands, and you mentioned "I have reverted to this approach",

but then I looked at the code and there are a few snippets that look like this:

   if is_rendering_required(" ".join(others)):
        expand_args(args, user_ns)

the objective is to have a single place for expanding the {{variables}} and I thought we had agreed to do that. but it seems like we still have an implementation that requires us adding the same snippet on each command?

[neelasha23]

the objective is to have a single place for expanding the {{variables}} and I thought we had agreed to do that. but it seems like we still have an implementation that requires us adding the same snippet on each command?

What i understood from your previous comment, you mentioned that we we might forget to add every-time we add a new argument so we need a approach where we dont need to bother abut adding the arguments to a list. But in the current approach we dont need to maintain any list of that sort it automatically detects the "{{value}}" pattern inside argparse and replaces them.
But we still need to call this util function at the different commands file right? like sql, sqlplot. and the different versions of sqlcmd. Also, sqlcmd is parsed differently than than the other two.

Now if we add any new command like sqlcmd something or sqlsomething only then we need to add this block of code otherwise not. Does this make sense?

the objective is to have a single place for expanding the {{variables}} : I'm also not sure of this part either because even if we expand the string we would need to do it separately for sqlplot, sqlcmd and sql right ? because the line text will be read in that particular command file only. It would be there at 3 files. (And with this appraoch we have the problem where user might try to declare variables for any token and not just parameters) .But with current approach we need to include it in all the sqlcmd files because there are separate parser files like snippet, columns, etc. (only for sqlcmd)

If we work directly on the string we need to add multiple rules/ regex patterns to ensure we pick up only the arguments and I'm not very sure of this approach.

Let me know what you think. If you have any suggestions it would be helpful

@edublancas

[edublancas]

@neelasha23 oh, you're right. I thought we could add a single piece of logic that would work for all commands (%%sql, %sqlcmd , etc). But then I realized that they are all implemented in different places as they are magic objects, so your approach makes sense.

just fix the conflicts, the integration tests and this should be good to go!

[neelasha23]

conflicts and tests are fixed now @edublancas