fix: validate attribute types at declaration and insert (#1527, #1528, #1529, #1530) - #1531
fix: validate attribute types at declaration and insert (#1527, #1528, #1529, #1530)#1531dimitri-yatsenko wants to merge 3 commits into
Conversation
…#1529, #1530) Four bugs with one shape: the type system accepted spellings it could not honour, and failed late — at the driver or the server — instead of at declaration. #1527 A native blob column accepted any object. DataJoint passed it to the driver untouched; PyMySQL has no encoder for ndarray and falls back to str(value), so an array was stored as its text repr, elided in the middle for large arrays, with no error on insert or on fetch. __make_placeholder now requires bytes for a codec-less blob attribute. The declaration warning for native blob types now says the column stores raw bytes and names <blob>, rather than the generic portability note. #1528 decimal(M,D) carrying `unsigned` or `zerofill`, spelled `dec`/`fixed`, or given a single argument was rejected outright, where 0.14.x accepted it. 2.x promoted decimal to a core type with a strict pattern and left the old permissive branch under the name NUMERIC, which kept `numeric` and dropped `decimal` — so `numeric(2,2) unsigned` passed while the canonical spelling of the same SQL type did not. NUMERIC covers all four aliases again; the core DECIMAL pattern still wins for decimal(M,D). Heading derives its `numeric` flag from NUMERIC too, so a modified decimal is no longer classified as neither numeric nor string. #1529 NATIVE_TO_CORE_TYPE mapped unsigned integers to uint8/uint16/uint24/ uint32/uint64 and mediumint to int24, none of which exist as core types. Phase 2 migration wrote :uint32: markers that Heading then silently dropped. Unsigned columns now widen to the next signed type that holds their range, as the migration guide already documented. bigint unsigned has no lossless target and logs a warning. #1530 match_type used re.match, and the $ in the INTEGER pattern bound to the `serial` alternative only, so `int24`, `tinyinteger` and `intbanana` all classified as native integers and were emitted into the DDL verbatim. Now fullmatch, with the INTEGER alternation anchored as a whole. Also: bare `blob` is a MySQL type and was rejected while the sized variants were accepted, though migrate.BLOB_TYPES already recognized it. Found by the new cross-reference test. tests/unit/test_type_patterns.py asserts that every type named in NATIVE_TO_CORE_TYPE round-trips through match_type, that near-miss spellings raise, and that the native spellings still accepted are not regressed by the strictness change.
The migration guide promised `decimal(M,D) → decimal(M,D) # unchanged`, which is true only of the canonical two-argument form. The same SQL type carrying a modifier or spelled with an alias is a native type, not a core type, and until datajoint/datajoint-python#1528 it was rejected outright — so readers with an unsigned decimal hit an error immediately after reading that it was unchanged. - state that decimal is a core type only as decimal(M,D), and that decimal(M,D) unsigned / decimal(M) / numeric / dec / fixed pass as native - complete the unsigned-integer table: add mediumint, and the signed rows that were implied but missing - call out bigint unsigned as the one lossy widening, since values above 2**63-1 do not fit in int64; dj.migrate warns per column - add bare `blob` and `tinyblob` to the codec mapping — both are MySQL types and both migrate to <blob> Documents the behaviour of datajoint/datajoint-python#1531.
MilagrosMarin
left a comment
There was a problem hiding this comment.
Verified the four fixes. The fullmatch change is safe — match_type() is only reached from declare.py:857/:957, both on the declaration path, never on server-reported types, and the patterns involved already end in $. The insert guard correctly gates on attr.is_blob and attr.codec is None.
On the bigint unsigned call you flagged — traced it through:
migrate_columns emits MODIFY COLUMN {native_type} … COMMENT, so the physical column is preserved and stored data is untouched; values above 2**63-1 still fetch fine. The effect is descriptive: heading.py:517 sets original_type = "int64", and Table.describe() renders original_type or attr.type — so describe() reports int64 for a column that is physically bigint unsigned.
That's where it bites: the describe→recreate→copy path produces a signed bigint, and values above 2**63-1 don't survive that copy. Nothing is lost at migration time, and the warning is well-placed, but the label is what describe() echoes downstream.
Worth weighing as an alternative: leave bigint unsigned out of NATIVE_TO_CORE_TYPE and give it an explicit warn-and-skip branch. The column then stays honestly native and describe() keeps reporting bigint unsigned. It would need the explicit branch — unmapped types currently fall through the "silently skipped" path, and silence would be worse than what you have now. Your call; both are defensible, but the describe() round-trip is the consequence I'd weigh it against.
Smaller: the new heading.py comment says prefix matching is what lets PostgreSQL spellings like "double precision" and "timestamp without time zone" match on their leading word. Testing both directly, re.match returns None for each — FLOAT and TEMPORAL both end in $, and every pattern in those tuples is anchored, so .match() and .fullmatch() are equivalent there. No runtime effect; just a rationale future maintainers would take at face value.
…-trip) and the lossless escape hatch (decimal(20,0)); fix the match-vs-fullmatch comment
|
Addressed the review:
|
Closes #1527, closes #1528, closes #1529, closes #1530.
Four bugs with one shape: the type system accepted spellings it could not honour, and
failed late — at the driver or at the server — instead of at declaration. Fixing them
separately would have meant four passes over the same twenty lines, so they are here
together.
#1527 — native blob silently stored
str(array)__make_placeholderhanded the value to the driver untouched for a codec-less blobattribute. PyMySQL has no encoder for
ndarrayand falls back toescape_str, so anarray was stored as its text repr — elided in the middle for anything large — with no
error on insert and none on fetch. It now requires
bytes/bytearray/memoryviewand names
<blob>in the error. This is the one with actual data loss.The declaration warning for native blob types also now says the column stores raw
bytes and points at
<blob>, instead of the generic "consider a core DataJoint typefor better portability", which reads as cosmetic.
Note the failure was MySQL-specific: on PostgreSQL the same insert already raised
can't adapt type, because that adapter registers numpy scalars only. The guard makesthe two backends agree.
#1528 —
decimal(M,D) unsignedrejected outrightA regression from 0.14.x, which had one permissive pattern for both spellings. 2.x
promoted
decimalto a core type with a strict pattern and left the old permissivebranch under the name
NUMERIC— which keptnumericand droppeddecimal. Theresult was that
numeric(2,2) unsignedpassed while the canonical spelling of thesame SQL type raised.
NUMERICnow coversdecimal/numeric/dec/fixedwithunsigned/zerofill; the coreDECIMALpattern is still matched first, sodecimal(M,D)remains a core type.Headingderives itsnumericflag from these same patterns and did not consultNUMERIC, so a modified decimal was classified as neither numeric nor string. Fixedin the same place.
Found in
element-optogenetics, which declares fourdecimal(2, 2) unsignedproportions and cannot be declared at all on 2.x.
#1529 — migration wrote core-type markers that do not exist
NATIVE_TO_CORE_TYPEmapped unsigned integers touint8…uint64andmediuminttoint24, none of whichmatch_typeaccepts. Phase 2 stamped:uint32:into columncomments and
Headingthen silently dropped the marker, so the effect was cosmeticrather than destructive — but the labels meant nothing, and the guide documented a
different mapping than the code performed.
Unsigned columns now widen to the next signed type that holds their full range, which
is what
migrate-to-v20.mdalready said.bigint unsignedis the one case with nolossless target — values above 2**63-1 do not fit in
int64— so it maps toint64and logs a warning naming the column. That is the decision most worth a secondopinion in this PR.
#1530 — misspelled types classified as native integers
match_typeusedre.match, and the$in theINTEGERpattern bound to theserialalternative only, soint24,tinyintegerandintbananaall matched ontheir prefix and were emitted into the DDL verbatim — producing a MySQL syntax error
instead of
Unsupported attribute type. Nowfullmatch, with the alternation anchoredas a whole.
tests/integration/test_declare.py::test_unsupported_int_datatypealready assertedthis behaviour and was passing only because the server rejected the DDL; it now passes
for the right reason.
I did not move the
TYPE_PATTERNcall sites inheading.pytofullmatch, which#1530 suggested. Those match against types reported by the server, which are not
normalized — PostgreSQL returns
double precisionandtimestamp without time zone,and both currently classify on their leading word.
fullmatchthere would break thePostgreSQL backend. Normalizing adapter-reported types is the real fix and is a larger
change; I left a comment at the call site saying so.
Also: bare
blobNATIVE_BLOBrequired a size prefix, soblob— a MySQL type in its own right, andone
migrate.BLOB_TYPESalready recognized — was rejected whiletinyblobandlongblobwere accepted. The size prefix is now optional. This was found by the newcross-reference test rather than by reading, and it is real:
element-moseqdeclaresthree bare
blobattributes.Testing
tests/unit/test_type_patterns.pycovers the seam between the three places that namea type. The assertions that would have caught these:
NATIVE_TO_CORE_TYPEround-trips throughmatch_type(migrate.py stamps :uint32:/:int24: markers for core types that do not exist; guide documents a different mapping #1529, bare blob)strictness change cannot quietly narrow what is accepted
Plus integration tests for the insert guard, a bare
blobround-trip, and declaringand reading back
decimal(2, 2) unsigned.Full suite green on both backends: 1021 passed, 10 skipped.
ruff,ruff-formatandmypyclean at the versions pinned in.pre-commit-config.yaml.Follow-up
The documentation half needs a
datajoint-docsPR:migrate-to-v20.md:1180currentlypromises
decimal(M,D) → decimal(M,D) # unchanged, and:1080documents theunsigned-integer mapping this PR aligns the code to.
type-system.mdshould also saywhat happens to native modifiers. Not in this repo, so not in this PR.