BUG: query modifies the frame when you compare with `=`
State: closed

Fix for #8664. The simplest way to fix this involved adding an `assignment_allowed=True` arg to the internal `eval()` function. All existing behaviour should be preserved. If adding the arg isn't okay I'm not quite sure how else to do it, as it's only once we reach the internal eval function that the expression is actually parsed and we know that it includes the assignment.

It also seems like a pretty bad side effect if query can silently overwrite values (obviously only if you accidentally include an assignment), so hopefully that's a good argument in favour of having the
`assignment_allowed` arg.


Labels:
name: Bug
name: API Design

Comments:
This is much simpler actually.

The `res` is `None` here because the query didn't return anything (e.g. it was an assignment). This is allowed.
So only do the following try: except if `res is not None`

https://github.com/pydata/pandas/blob/master/pandas/core/frame.py#L1918

Separate issue if we don't want to allow assignment in a `.query` (it IS allowed in `.eval`). 
Fixing that is a bit more complicated as currently `query` is implemented in terms of `eval`. And they use the same expression node-set (e.g. they parse to the same thing and have the same unsupported nodes and such). So I think you would have to create a new dialect (maybe `parser='pandas_query'`) which is a derivative of `parser='pandas'`.

This is not that difficult actually, just sub-class and add an unsupported node. (and add it to the tests for the various dialects).

@cpcloud 

One thing to try is adding an unsupported node at runtime (instead of at class definition time which is how it's done now) based on the suggested flag

@onesandzeroes you want to give a shot to modify as discussed above? thxs

@jreback Having another go at this now with a new parser subclass. I tried the simpler method you suggested of checking if `res` was `None` but I don't think that works as the assignment/mutation of the dataframe (which shouldn't ever happen from a query, IMO) has already happened at that point.

If the parser subclass looks like the right way to go I can clean up so that the `PandasQueryExprVisitor` isn't just a copy-paste of the original `PandasExprVisitor`. Obviously need to go through and clean up different docstrings etc. as well. Just slightly slow going at the moment as a lot of this is pretty new to me. 

Doing this does make attempted assignment from `query` raise, although the error message is
a bit cryptic- you get:

``` python
df.query('a=1')
Traceback (most recent call last):
...
NotImplementedError: 'Assign' nodes are not implemented
```

So I guess we want to catch the exception and give a more specific message.

Another option I just thought of is that we can just override `visit_Assign` in the query parsers, which makes it nice and clear that we shouldn't ever do anything with an assignment from a query. Ends up looking pretty clean, it's basically:

``` python
class PandasQueryExprVisitor(PandasExprVisitor):

    def visit_Assign(self, node, **kwargs):
        raise ValueError("Cannot assign within queries")
```

@onesandzeroes I like that latest idea!

@onesandzeroes want to rebase and we can re-examine this for 0.17.0?

can you rebase?

OK, I rebased. I feel like this one is probably above my paygrade and I'm obviously not finding time to work on this stuff at the moment, so if anyone else want to take over they should definitely go ahead.

I do still have a branch sitting around where I tried to do this more simply by subclassing the existing `ExprVisitor` classes and overriding `visit_Assign`, it seemed promising at first but there were a lot of test cases to rewrite and I'm not sure if it was ultimately going to work.

@onesandzeroes what you are doing looks reasonable.

Need to explicity pass the parser though to have it catch the assign nodes (your tests catches it, but a real world example will still fail). e.g. in `DataFrame.query` 

- can you rebase
- add a whatsnew note for 0.17.1 (add in API changes, give a mini example)
- squash

@onesandzeroes can you update according to comments

@onesandzeroes pls update according to comments

can you update

closing in favor of #11149 ; this is fixed more generally