Make arrays of several integer types adaptable - #147
Open
roed-math wants to merge 2 commits into
Open
Conversation
psycopg dumps a list with one dumper for the whole array, and refuses a
list whose entries have different types unless those types share a
Postgres oid. Sage's Integer dumps with oid 0 (unknown, so the server
infers from context) while Python's int dumps as int2/int4/int8/numeric
by magnitude, so any query list assembled from both -- a computed value
here, an int literal there -- raised
psycopg.DataError: cannot dump lists of mixed types; got: Integer, int
Under psycopg2's client-side interpolation such a list worked, so the
driver port turned every call site that builds one into a 500. Two are
known in LMFDB: the CM field lookup on elliptic curve isogeny class
pages (fixed there in LMFDB #7129/#7130) and the L-function Euler
product search, which pads a Sage coefficient list with int zeros
whenever the entered Euler factor has less than the searched degree.
_execute now makes such a list homogeneous before binding it, which is
the one place every statement psycodict runs passes through. Only lists
that would otherwise have failed are touched: a list of a single type is
left alone whatever that type is, as is one holding a non-integer, one
holding a bool (which Python counts as an integer and Postgres does
not), and one whose types psycopg already knows -- notably its own
Int2/Int4/Int8/IntNumeric wrappers, which subclass int precisely to pin
down an oid. A nested array is flattened first, since it is one array
to Postgres and psycopg compares its entries throughout.
Json.prep gains the matching fallback: psycodict serializes the query
itself to json for the statistics cache, so a value type accepted in a
query has to be encodable there too. It sits after the str/bool/float/
int branch so that a bool stays a bool.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documentation build overview
4 files changed± genindex.html± api/encoding.html± _modules/psycodict/base.html± _modules/psycodict/encoding.html |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
psycopg picks one dumper for a whole array, and refuses a list whose entries have different types unless those types dump to the same Postgres oid.
SageIntegerDumperhas oid 0 (unknown, so the server infers the type from context, deliberately mimicking what psycopg2's client-side interpolation did) while Pythonintdumps as int2/int4/int8/numeric by magnitude, so the two never agree:Homogeneous lists are fine either way, so this only bites a caller that assembles an array out of more than one source: a computed value here, an int literal there. Under psycopg2 such a list worked, so the driver port turned every one of those call sites into a 500 without any of them changing.
Two are known in LMFDB. The CM field lookup on elliptic curve isogeny class pages was found in production and patched at the call site in LMFDB#7129. The L-function Euler product search is the second, found while writing this and confirmed against the LMFDB mirror:
parse_eulerpads a Sage coefficient list with Python int zeros, so any entered Euler factor of less than the searched degree (F2=1-Tat degree 4, say) fails. Neither is a psycodict bug exactly, but the failure mode is invisible until the untested branch runs, which makes it worth a net rather than another round of call-site fixes.The fix
homogenize_int_arraysinencoding.py, applied to a statement's values from_execute— the one place every statement psycodict runs passes through, so searches, counts, statistics, inserts and updates are all covered by the single call.It is deliberately narrow: a list is only rewritten when it would otherwise have raised. There must be at least two distinct element types, every one of them integral, none of them a
bool(which Python counts as an integer and Postgres does not), and at least one of them not anintsubclass. That last condition leaves psycopg's ownInt2/Int4/Int8/IntNumericwrappers alone: they subclassintprecisely to pin down an oid, and replacing them with plain ints would throw away the choice. So no query that works today changes behaviour, andSageIntegerDumperkeeps its oid 0 — giving it a real oid would have been the other way to make the types agree, but it would break a SageIntegercompared against atextcolumn, which oid 0 handles today.A nested array is flattened first, since Postgres and psycopg both treat it as a single array whose entries must agree throughout rather than row by row.
When nothing needs fixing the values come back as the same object, so ordinary queries copy nothing.
Two things only came out of testing:
insert_manybinds named parameters (%(col)s) with dict rows, so the values walker has to handle mappings and not just sequences. The first version silently did nothing there.quick_countserializes the query itself to json for the statistics cache, so a value type accepted in a query has to be encodable there too.Json.prepgets the matchingnumbers.Integralfallback, placed after the(str, bool, float, int)branch so that aboolstays abool.Tests
tests/conftest.pygrows aFakeIntegerregistered undernumbers.Integral— psycodict does not depend on Sage, so the type this exists for cannot be imported by the unit jobs, and registering reproduces everything the code and psycopg look at. Covered: the conversions, everything deliberately left alone, each of psycopg's oid wrappers, both values shapes, the no-copy property, and a Sage-mode test using the realIntegeron the shape that broke the isogeny class pages. Round-trip tests against a real database go through flatinteger[]andnumeric[]columns, a two-dimensional column, the array operators,insert_manyandupdate.Full suite green under Sage and without it, ruff clean. Verified by hand against the LMFDB mirror: the CM field lookup, the Euler product search and a nested
integer[]column all fail on 1.0.0rc2 and pass on this branch.