Add function to clean up column names with special characters
State: closed

Changed the backtick quoting functions so that you can also use backtick quoting to use invalid Python identifiers like ones that start with a digit, start with a number, or are separated by operators instead of spaces. 

- [x] closes #27017
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

As this builds upon #24955 I think @jreback would be again the right person for the code review.

Labels:
name: DataFrame
description: DataFrame data structure

Comments:
I don't know how I can solve this issue `pandas/core/computation/common.py:4: error: Module 'tokenize' has no attribute 'EXACT_TOKEN_TYPES'` as the tokenize module has this attribute. (In the "Type validation" check.)
https://docs.python.org/3/library/tokenize.html#module-tokenize does not refer to EXACT_TOKEN_TYPES and typeshed does not have a definition for EXACT_TOKEN_TYPES https://github.com/python/typeshed/blob/master/stdlib/3/tokenize.pyi

my guess is it's not part of the public API for tokenize.
@simonjayhawkins I see. Shall I write out this dictionary instead of importing it?
just reading the docs.. "All constants from the token module are also exported from tokenize." https://docs.python.org/3/library/tokenize.html#tokenizing-input

so it would perhaps appear that the omission of EXACT_TOKEN_TYPES is an oversight in typeshed.

so probably best to add `# type: ignore` and a comment.
> just reading the docs.. "All constants from the token module are also exported from tokenize." https://docs.python.org/3/library/tokenize.html#tokenizing-input
> 
> so it would perhaps appear that the omission of EXACT_TOKEN_TYPES is an oversight in typeshed.
> 
> so probably best to add `# type: ignore` and a comment.

forget that. I misread the docs, 

from the source..

```
import token
__all__ = token.__all__ + ["tokenize", "detect_encoding",
                           "untokenize", "TokenInfo"]
```
can you use tokenize.tokenize instead? The returned named tuple has an additional property named exact_type that contains the exact operator type for OP tokens. For all other token types exact_type equals the named tuple type field.
@simonjayhawkins 
Well, I basically need a list of the operators first, and than I need the number associated with that operator. I actually don't know how exactly I would do that like that. 

So, I guess, I either ignore, or write them out manually. 
Changed code so that the PR now also closes #28576. 
EDIT: I just moved all to expr.py. Made more sense. 

Also, I moved some more functions from pandas/core/computation/expr.py to pandas/core/computation/common.py. But the latter isn't anymore a good place for that. It is probably better to have them moved all to expr.py again, but maybe it is also a good idea to put them in a new file backtickquoting.py? (About 100 lines)
Implemented code that raises an exception if there is ambiguity. (Ambiguity should be very rare.)

Also added an exception if there somehow exists a way the conversion fails to create a valid Python identifier. (Currently cannot think of one.)

Also moved some functions around.

Probably need a new code review. @WillAyd you want to take a look again?
@WillAyd What do you think of it now. (I am not entirely sure why travis fails, it says something about TestS3, so maybe a (temporal) problem with AWS?)
@WillAyd What do you think. Is it ready already, and should I maybe to a squash or something?
@WillAyd No need to apologize. I know pandas is a very popular Python package. So, it is good to take the time for this.  
Well, I had some doubt over that filter / map thingy. Usually the comprehensions are favored over the those, and the `__contains__` isn't so nice either. But for people that are a bit more familiar with functional programming I think it reads a lot better than the previous version. 
    
I have changed the doc, but I will wait for the push. (Don't want to put too much work on your automated pipelines.) As I expect to still have some more improvements and probably also squash the commits etc.
@jreback 

This PR does not only add the functionality of using special characters in backtick quoted identifiers. It improves on a wide range of issues with my latest PR (#24955). 

To summarize, this PR consists of the following improvements:

* Raise an error when it is not possible to convert a backtick quoted string to a valid Python identifier. (These cases are rare, but it is good to still raise an understandable error.) (`_create_valid_python_identifier`)
* Raise an error when the conversion of the column resolvers to valid Python identifiers creates an ambiguity. This has to do with how Python parses spaces. This issue was already in there with the previous PR, but wasn't handled. Although I doubt someone in the world would ever differentiate its column names based on a difference in whitespace, it is still good to have such an error raised, as it would otherwise pass silently. (`_get_space_character_free_column_resolvers`)
* Moved all code from common.py to expr.py. (I think it is better to have that at one place, I realized.)
* The actual improvement of using special characters in the backtick quoted strings. (`_remove_spaces_column_name` -> `_create_valid_python_identifier`)
* Tests to cover the above.
* The backtick quoted string as well as the column name will undergo the same process (tokenizing and cleaning) so that special cases where tokenizing removes too much whitespace, still get resolved in the end. (see for example #28893)
* Improved documentation that also covers the edge cases.

I think the PR improves on a lot of things, and not only adding more coverage for different cases for the backtick quoted strings. 

Also, keep in mind that we are not expanding the DSL for the .query function. We are only adding more coverage for different cases in naming, i.e. special characters, next to spaces. 
Yet another improvement. I was able to fix the problem with the multiple spaces. I just made use of the indices of the token_generator to extract the exact indices of the backtick quoted string. So, you completely circumvent any undesired manipulation done to the tokens. 

This will simplify the PR also because it removes documenting that edge case and catching an error when that occurs (that was a significant portion of code.).
@jreback Can you do the codereview?

Current failed test if just some linting in an unrelated file. 
@pandas-dev/pandas-core if others can have a look here
Sorry if this is a silly question @hwalinga, I understand the goal of this is to be able to run something like:
```python
import pandas
df = pandas.DataFrame({'$ amount': [10, 200, 20]})
df.query('`$ amount` < 50')
```
And what you're doing is converting `$ amount < 50` to `_DOLLARSIGN__amount > 50` before the `eval` is executed.

Why don't we execute the `eval` as `locals()['$ amount'] < 50` instead?
@datapythonista 

An interesting observation; I never thought about that. However, upon trying it, I got the an error saying that the function `locals` is not supported by the `query` function. 

I don't know the exact workings of the downstream code, but I think such a solution in which we try to support locals for downstream code might become too complex, also because there are different engines by which the query can be executed. There is python and numexpr, and I don't know what will happen in the future. If more engines will be added we need to support locals for that as well, while this solution will work regardless. 

Therefor, in my view, albeit the current solution is a bit hacky, it does provide a simpler query string for any downstream code working with it. 
> @datapythonista
> 
> An interesting observation; I never thought about that. However, upon trying it, I got the an error saying that the function `locals` is not supported by the `query` function.
> 
> I don't know the exact workings of the downstream code, but I think such a solution in which we try to support locals for downstream code might become too complex, also because there are different engines by which the query can be executed. There is python and numexpr, and I don't know what will happen in the future. If more engines will be added we need to support locals for that as well, while this solution will work regardless.
> 
> Therefor, in my view, albeit the current solution is a bit hacky, it does provide a simpler query string for any downstream code working with it.

@hwalinga so the point is this. we *don't* need to actually use numexpr here for non-trivial usages. we can simply execute it directly (this in fact is what ``engine='python'``. I am all for reorganizing the code, but I agree it would be nice to actually simplify this. 
So, the proposal is to move the name resolving to the ast tree instead (the parsing used by engine='python'). The ast parser is a very complicated piece of code and there is already a lot of custom stuff in there. The tokenization is much easier and there is not so much custom stuff in that. I think it is not a good idea to move some of the functionality to the ast parser, and here I summed up why:

 * I don't think it is a good idea to bring more customization to the already complex tree parser.

 * I think it is better to keep the code regarding backtick quoting clustered and not to scatter it around with some in the token parsing and some in the tree parsing. I like that is now all in one place in the `parsing.py` file.

 * Backtick quoting is more related to the token parsing. The code now just simplifies the query before it got send to the engine. (It resolves the backtick quoting, and updates the name resolvers.)

 * I think pandas should simplify the query the most as possible before it sends it to the engine, because the engine could be a third party engine. (Like the 'numexpr'.) This will probably also help in the future whenever there is the possibility to add more engines. You want to keep the amount of features of the query that they are processing to a minimum, so that those engines can focus primarily on the processing than on the parsing. So, they only deal with a mostly simple query in addition to the name resolvers, not too much other stuff.

 * Numexpr does not support locals()[]. We can use such a thing only for our own custom parser and that will make this feature only available for engine='python'. This might be not a very big of a deal (most people probably just use the python engine and don't bother with installing numexpr), and I agree we don't have to write overly complicated code to support both. However, we already support both with the current implementation and since 'numexpr' is the default engine it also would not really make sense to not support that one. (It however autofallbacks to python, which I also don't think is a very good idea, but I elaborated on that already in #30005)

 * This would also mean a regression, as 25.3 already supports backtick quoting for both numexpr and python already.

With all this in mind, I think we should just proceed with the current implementation we have now. 

> the possibility to add more engines

Is this something we actually anticipate happening?  
@jbrockmendel 

I think we don't expect new engines to be added, but with this statement in the readme `Additionally, it has the broader goal of becoming the most powerful and flexible open source data analysis / manipulation tool available in any language.`, I thought query might play a role in that. So, that is just one of the reasons I wanted to keep the query that is send to the engine as simple as possible. 
Test failures unrelated to this PR. 
> Test failures unrelated to this PR.

if you merge master these should be fixed
Thanks, I thought that would be done automatically. My bad.
Another unrelated failed test:

```
pyarrow.lib.ArrowInvalid: Could not convert NA with type NAType: tried to convert to boolean
```
> Another unrelated failed test:

That should be fixed in master
@hwalinga responses inline

> * I don't think it is a good idea to bring more customization to the already complex tree parser.

actually the ast parser is what python uses, i am not sure we are expanding this *at* all.

> * I think it is better to keep the code regarding backtick quoting clustered and not to scatter it around with some in the token parsing and some in the tree parsing. I like that is now all in one place in the `parsing.py` file.

yes I agree with this, your code-org is nice

> * Backtick quoting is more related to the token parsing. The code now just simplifies the query before it got send to the engine. (It resolves the backtick quoting, and updates the name resolvers.)
> * I think pandas should simplify the query the most as possible before it sends it to the engine, because the engine could be a third party engine. (Like the 'numexpr'.) This will probably also help in the future whenever there is the possibility to add more engines. You want to keep the amount of features of the query that they are processing to a minimum, so that those engines can focus primarily on the processing than on the parsing. So, they only deal with a mostly simple query in addition to the name resolvers, not too much other stuff.

this is true, but you are missing the points. the *only* engine that is supported is ``numexpr`` which must have strings. I don't think in *pandas* we will *ever* have another engine, The simple soln is simply to fallback to actual python execution where columns names can be *anything*.

> * Numexpr does not support locals()[]. We can use such a thing only for our own custom parser and that will make this feature only available for engine='python'. This might be not a very big of a deal (most people probably just use the python engine and don't bother with installing numexpr), and I agree we don't have to write overly complicated code to support both. However, we already support both with the current implementation and since 'numexpr' is the default engine it also would not really make sense to not support that one. (It however autofallbacks to python, which I also don't think is a very good idea, but I elaborated on that already in #30005)

this has nothing to do with numexpr, all variable references are resolved prior to sending to the engine

> * This would also mean a regression, as 25.3 already supports backtick quoting for both numexpr and python already.

no it wouldn't, this is an extension of parsing.