pypika icon indicating copy to clipboard operation
pypika copied to clipboard

`isin` method should not use narrow isinstance for generating criterion

Open JordanReiter opened this issue 3 years ago • 1 comments

This is the code:

def isin(self, arg: Union[list, tuple, set, "Term"]) -> "ContainsCriterion":
    if isinstance(arg, (list, tuple, set)):
        return ContainsCriterion(self, Tuple(*[self.wrap_constant(value) for value in arg]))
    return ContainsCriterion(self, arg)

You'll notice that if it is anything other than a list, tuple, or set, the code assumes it is already a type that the query builder can handle "natively" without conversion.

If you send it an iterable other than the ones specified above, you will get a failure later on, namely:

AttributeError: <object type> object has no attribute 'nodes_'

In my case I was using:

....where(col.isin(someval.keys()))

And got the error

AttributeError: 'dict_keys' object has no attribute 'nodes_'

But this could just as easily happen with someval.values() or even some custom-built iterable used in the code.

Unless I'm missing something, the desired input for ContainsCriterion would always be a Tuple object so perhaps a better implementation would be:

def isin(self, arg: Union[list, tuple, set, "Term"]) -> "ContainsCriterion":
    if isinstance(arg, Tuple):
        return ContainsCriterion(self, arg)
    return ContainsCriterion(self, Tuple(*[self.wrap_constant(value) for value in arg]))

Just a thought. If Tuple is not always the right input, then some class that implements a base class of Tuple should be used in isinstance. Limiting the inputs to one of three types (common as they may be) leads to unexpected errors such as the one I listed above.

They may even be other methods that use this isinstance check, this is just the only one I found.

JordanReiter avatar Feb 11 '22 16:02 JordanReiter

I guess isin() function should also understand *args i.e. table.column.isin('value1', 'value2')

doc-sheet avatar Mar 26 '23 16:03 doc-sheet