New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The adaption of nested empty list in `2.7.5` #788

Closed
iamalbert opened this Issue Oct 10, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@iamalbert

iamalbert commented Oct 10, 2018

The adaption of empty list is different in 2.7.4 and 2.7.5

The result of 2.7.5 will cause malformed array literal if it used with =ANY(:arr)

>>> import psycopg2; psycopg2.__version__
'2.7.4 (dt dec pq3 ext lo64)'
>>> from psycopg2.extensions import adapt, register_adapter, AsIs
>>> adapt([]).getquoted()
b"'{}'"
>>> adapt([[]]).getquoted()
b"ARRAY['{}']"
>>> adapt([[[]]]).getquoted()
b"ARRAY[ARRAY['{}']]"
>>> import psycopg2; psycopg2.__version__
'2.7.5 (dt dec pq3 ext lo64)'
>>> from psycopg2.extensions import adapt
>>> adapt([]).getquoted()
b"'{}'"
>>> adapt([[]]).getquoted()
b"'{{}}'"
>>> adapt([[[]]]).getquoted()
b"'{{{}}}'"

fogzot added a commit that referenced this issue Oct 10, 2018

@fogzot

This comment has been minimized.

Member

fogzot commented Oct 10, 2018

I just added a test for this regression: 1fe9f1a

fogzot added a commit that referenced this issue Oct 10, 2018

@dvarrazzo

This comment has been minimized.

Member

dvarrazzo commented Oct 10, 2018

For reference, the change was needed to fix #325.

@dvarrazzo dvarrazzo added this to the psycopg 2.7.6 milestone Oct 15, 2018

@dvarrazzo

This comment has been minimized.

Member

dvarrazzo commented Oct 23, 2018

Observation 1: the previously returned value was wrong:

=# select ARRAY['{}'];
 {"{}"}
=# select ARRAY[ARRAY['{}']];
 {{"{}"}}

This is an array containing the string {}.

Observation 2: postgres just doesn't do arrays of empty arrays:

=# select ARRAY['{}'::text[]];
 {}
=# select ARRAY[ARRAY['{}'::text[]]];
 {}

at this point I have no idea how we are supposed to fix this bug :\

@fogzot

This comment has been minimized.

Member

fogzot commented Oct 23, 2018

We can determine if the variable is just a nest of empty arrays and send a single one to PostgreSQL. We lose round-tripping but the value is lost anyway.

@dvarrazzo

This comment has been minimized.

Member

dvarrazzo commented Oct 23, 2018

=# select array[array[]]::text[][] = array[]::text[];
 t

This sucks so much... /cringe However it's valid syntax.

dvarrazzo added a commit that referenced this issue Oct 29, 2018

Fixed adaptation of lists of empty lists
...somehow. Postgres doesn't support them and converts them into a
simple empty array. However this is not really our concern: the syntax
we return is valid.

Close #788
@dvarrazzo

This comment has been minimized.

Member

dvarrazzo commented Oct 29, 2018

yet another observation the constructor: array[array[]] doesn't work unless it's cast to something:

piro=# select ARRAY[ARRAY[]];
ERROR:  cannot determine type of empty array
LINE 1: select ARRAY[ARRAY[]];
                     ^
HINT:  Explicitly cast to the desired type, for example ARRAY[]::integer[].

this is the same failure of ARRAY[]. Because adapting an empty list is quite desirable we special-case it returning the literal '{}', which I guess it's interpreted as an empty array of unknown in the right context, e.g. in any() (or of the type on the other side of the operator. don't know. don't care).

So, even the test proposed doesn't work:

curs.execute("select null = any(%s)", ([[]], ))
self.assertFalse(curs.fetchone()[0])

this fails because it requires a cast next the placeholder. But it sucks because normally you wouldn't require such a cast: array of non-array don't need it, so you risk a statement working with some values (non-empty lists, empty lists) and failing with some peculiar ones (lists of empty lists). On the other end, if someone has a python list of uniform objects, e.g. ints, I don't expect them to put an empty list into it.

In other words, if the fix-788 branch doesn't show other regressions, I'm ok fixing it this way.

dvarrazzo added a commit that referenced this issue Oct 30, 2018

Fixed adaptation of lists of empty lists
...somehow. Postgres doesn't support them and converts them into a
simple empty array. However this is not really our concern: the syntax
we return is valid.

Close #788

@dvarrazzo dvarrazzo closed this in a83696f Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment