feat: support WHERE x IN (list) clauses with multiple items in list #352

gfieremans · 2023-04-07T23:13:14Z

Summary

Up to now, the constraintargs for a WHERE IN clause with more than one element in a list comes in as None in the VTCursor.Filter() method. With the use of the VTTable.BestIndexObject() and using apsw 3.41.0+, it is possible to get this list passed in as a set to the VTCursor.Filter() method. This PR serializes that set to a string.

Testing instructions

You will probably need to change the setup.cfg to set apsw>=3.41.0
set supports_requested_columns = True and supports_in_statements = True in your adapter's class derived from the Adapter base class..
make sure to properly handle the deserialization of a stringified set. A WHERE x IN (item) clause gets serialized to the string ==item, while the same clause with one more item WHERE x IN (item1, item2) gets serialized to the string IN("item1", "item2").

…thon 3.8

src/shillelagh/adapters/base.py

…ngle letter variables

src/shillelagh/backends/apsw/vt.py

cancan101 · 2023-05-24T14:45:44Z

src/shillelagh/backends/apsw/vt.py

+        if isinstance(constraint, set):
+            # A WHERE x IN (list) clause sends in constraintargs as a set when N>1:
+            # {item1,...,itemN}. We serialize this set to a string '["item1",...,"itemN"]'.
+            constraint = json.dumps(list(constraint))


ok, but why do you have to serialize this at all here?

Because these sets belong to an "Equals" clause and the current logic translates that for a string to '==string' and to leverage that principle the set needs to be converted to somethign like '==["string1", "string2"]'

I think the code should look something like this:

if isinstance(constraint, set): constraint = [type_map[column_type.type]().parse(c) for c in constraint] value = [column_type.format(c) for c in constraint] # operator = IN (not sure if eq is passed in here or if IN is set) else: constraint = type_map[column_type.type]().parse(constraint) value = column_type.format(constraint) all_bounds[column_name].add((operator, value))

ok yes, you will need to set the operator explicitly here to Operator.IN and create that operator as otherwise it will be EQ

OK - let me try that.

all_bounds[column_name].add((operator, value)) which value is a list so (operator, value) is unhashable

So change this line:

value = [column_type.format(c) for c in constraint]

To

value = tuple(column_type.format(c) for c in constraint)

Or use frozenset: https://docs.python.org/3/library/stdtypes.html#frozenset

Yes that should work, then we got adding new type of operator left, waiting for the update and merge ^^

Went with tuple() and added the Operator.IN

cancan101 · 2023-05-24T14:46:48Z

this also needs some tests

src/shillelagh/backends/apsw/vt.py

cancan101 · 2023-05-24T15:11:12Z

src/shillelagh/backends/apsw/vt.py

+        if isinstance(constraint, set):
+            # A WHERE x IN (list) clause sends in constraintargs as a set when N>1:
+            # {item1,...,itemN}. We serialize this set to a string '["item1",...,"itemN"]'.
+            constraint = json.dumps(list(constraint))


ok yes, you will need to set the operator explicitly here to Operator.IN and create that operator as otherwise it will be EQ

cwegener · 2023-05-26T09:33:42Z

I think the failing integration tests look unrelated to your work? And they were probably fixed here -> 48bb76a

You should just run pylint, black and mypy tests locally though and just address all those style and formatting checks.

gfieremans · 2023-05-26T09:36:45Z

Yes, thanks, I just figured that out too that the pre-commit is changing files. I think there still is one issue related to my PR though that I'm trying to track down. I seem to be changing the signature of requested_columns of VTTable.

cwegener · 2023-05-26T09:40:56Z

For pre-commit, just make sure it's installed on your local machine https://pre-commit.com/#install
And then run pre-commit install once off in your local shillelagh repo, and then all those local hooks will run before git commit runs (unless you use a different Git client ..... i.e. the hooks probably won't work with something like Eclipse Egit ...)

cwegener · 2023-05-26T09:46:43Z

. I think there still is one issue related to my PR though that I'm trying to track down. I seem to be changing the signature of requested_columns of VTTable

You should just be able to merge the latest main branch, or rebase onto main to incorporate the changes related to the API change. 14ed1f6

gfieremans · 2023-05-26T10:33:03Z

This particular error in my new test, I cannot figure out:

FAILED tests/backends/apsw/vt_test.py::test_virtual_best_index_object_with_in_statement - AttributeError: 'VTTable' object has no attribute 'requested_columns'

cwegener · 2023-05-26T10:43:56Z

This particular error in my new test, I cannot figure out:

FAILED tests/backends/apsw/vt_test.py::test_virtual_best_index_object_with_in_statement - AttributeError: 'VTTable' object has no attribute 'requested_columns'

The test runs successfully for my on Linux Python 3.9 .... 🤷 Something funny with the Github actions set up maybe ... I'm not a GH actions expert. 😉

src/shillelagh/backends/apsw/vt.py

cancan101 · 2023-05-26T14:43:47Z

src/shillelagh/backends/apsw/vt.py

-
-        all_bounds[column_name].add((operator, value))
+        if isinstance(constraint, set) and operator == Operator.EQ:
+            # See also https://rogerbinns.github.io/apsw/vtable.html#apsw.VTCursor.Filter


nit but I would write this as:

if isinstance(constraint, set) and operator is Operator.EQ: # See also https://rogerbinns.github.io/apsw/vtable.html#apsw.VTCursor.Filter constraint = [type_map[column_type.type]().parse(c) for c in constraint] value = tuple(column_type.format(c) for c in constraint) operator = Operator.IN else: constraint = type_map[column_type.type]().parse(constraint) value = column_type.format(constraint) all_bounds[column_name].add((operator, value))

That causes warnings because in the 'if' branch value is a tuple and in the 'else' branch it is a string.

You can solve that by forward declaring the type

value: Tuple | string # update type as needed if isinstance(constraint, set) and operator is Operator.EQ: # See also https://rogerbinns.github.io/apsw/vtable.html#apsw.VTCursor.Filter constraint = [type_map[column_type.type]().parse(c) for c in constraint] value = tuple(column_type.format(c) for c in constraint) operator = Operator.IN else: constraint = type_map[column_type.type]().parse(constraint) value = column_type.format(constraint) all_bounds[column_name].add((operator, value))

Is Any OK? I'd rather not get into a type cast war trying to list all possibilities

value: Any

Any is fine

src/shillelagh/filters.py

cancan101 · 2023-05-26T14:46:09Z

src/shillelagh/filters.py

+            return Impossible()
+
+        # we only accept tuples
+        value = values.pop()


why pop here?

Copied the concept from all the other filters. I'll remove the check for 'tuple' below and then normalize it with all the others.

@betodealmeida can comment on the pop and the rest of this code.

cancan101 · 2023-05-26T14:46:24Z

src/shillelagh/filters.py

+
+        # we only accept tuples
+        value = values.pop()
+        if not isinstance(value, tuple):


I would probably be looser here with the types accepted.

These come from https://rogerbinns.github.io/apsw/vtable.html#apsw.VTCursor.Filte. Only set is mentioned there I believe.

but that is tuple. Where does this get called from any ways?

The tuple? That's what we chose in

value = tuple(column_type.format(c) for c in constraint)

But I'm OK removing this constraint.

I am not totally clear where this code gets called. I remember it being cases where the sql engine doesn't handle filtering.

src/shillelagh/filters.py

betodealmeida

Sorry, this slid under my radar. Taking a look this weekend.

betodealmeida · 2023-08-19T19:24:49Z

src/shillelagh/adapters/base.py

+    # if true, a set from an IN statement will be assigned the new IN operator and
+    # passed in as a tuple to bounds in ``get_rows`` and ``get_data``. This requires
+    # supports_requested_columns to be set to true as well.
+    supports_in_statements = False


I'm not sure if we need explicit support at the adapter level. I think we might be able to just add the IN adapter, and if a given column doesn't support it we could replace it with a bunch of Equal operators.

gfieremans added 3 commits April 7, 2023 18:57

Rework serialization

202ea1c

Fix signatures of serialize_set() and deserialize_set() to support py…

769c448

…thon 3.8

deserialize_set() must return a set in case a single string is passed in

f4b84db

cancan101 reviewed May 24, 2023

View reviewed changes

src/shillelagh/adapters/base.py Outdated Show resolved Hide resolved

src/shillelagh/adapters/base.py Outdated Show resolved Hide resolved

src/shillelagh/adapters/base.py Outdated Show resolved Hide resolved

gfieremans added 4 commits May 24, 2023 02:46

fix set to list conversion and some linting issues regarding using si…

fcac9d2

…ngle letter variables

Remove special set serialization logic

0013220

Remove unused imports

33c5739

Fix linting issue

d001f48

cancan101 reviewed May 24, 2023

View reviewed changes

gfieremans added 5 commits May 25, 2023 08:15

Support IN Operator and add tests

4cb4ad3

check for EQ operator

c0495c9

Fix unit tests

17f602e

Fix comment in base.py

d3912e8

Windows tests behave different from CI tests, so fixing that

cc57518

gfieremans added 2 commits May 26, 2023 05:49

Merge branch 'main' into instatements

39931c1

pre-commit installed and address warning

54a96fa

cancan101 reviewed May 26, 2023

View reviewed changes

Address comments

a18d438

betodealmeida reviewed Aug 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support WHERE x IN (list) clauses with multiple items in list #352

feat: support WHERE x IN (list) clauses with multiple items in list #352

gfieremans commented Apr 7, 2023 •

edited

Loading

cancan101 May 24, 2023

gfieremans May 24, 2023 •

edited

Loading

cancan101 May 24, 2023 •

edited

Loading

cancan101 May 24, 2023

gfieremans May 24, 2023

rainnnnny May 25, 2023

cancan101 May 25, 2023

cancan101 May 25, 2023

rainnnnny May 25, 2023

gfieremans May 25, 2023

cancan101 commented May 24, 2023

cancan101 May 24, 2023

cwegener commented May 26, 2023

gfieremans commented May 26, 2023 •

edited

Loading

cwegener commented May 26, 2023

cwegener commented May 26, 2023

gfieremans commented May 26, 2023

cwegener commented May 26, 2023

cancan101 May 26, 2023

gfieremans May 26, 2023

cancan101 May 26, 2023

gfieremans May 26, 2023

cancan101 May 26, 2023

cancan101 May 26, 2023

gfieremans May 26, 2023

cancan101 May 26, 2023

cancan101 May 26, 2023

gfieremans May 26, 2023

cancan101 May 26, 2023

gfieremans May 26, 2023

cancan101 May 26, 2023

betodealmeida left a comment

betodealmeida Aug 19, 2023

feat: support WHERE x IN (list) clauses with multiple items in list #352

Are you sure you want to change the base?

feat: support WHERE x IN (list) clauses with multiple items in list #352

Conversation

gfieremans commented Apr 7, 2023 • edited Loading

Summary

Testing instructions

Choose a reason for hiding this comment

gfieremans May 24, 2023 • edited Loading

Choose a reason for hiding this comment

cancan101 May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cancan101 commented May 24, 2023

Choose a reason for hiding this comment

cwegener commented May 26, 2023

gfieremans commented May 26, 2023 • edited Loading

cwegener commented May 26, 2023

cwegener commented May 26, 2023

gfieremans commented May 26, 2023

cwegener commented May 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfieremans commented Apr 7, 2023 •

edited

Loading

gfieremans May 24, 2023 •

edited

Loading

cancan101 May 24, 2023 •

edited

Loading

gfieremans commented May 26, 2023 •

edited

Loading