Detect augmented JSON types when skipping batch migrate CAST - #1839
Open
ArockiaRajamanickam wants to merge 1 commit into
Open
Detect augmented JSON types when skipping batch migrate CAST#1839ArockiaRajamanickam wants to merge 1 commit into
ArockiaRajamanickam wants to merge 1 commit into
Conversation
The SQLite batch migration data transfer suppresses the CAST for JSON columns, since CASTing to JSON in SQLite yields 0. The check used isinstance(), which does not match a TypeDecorator that augments JSON, so the CAST was emitted and the column data was replaced with 0. Compare type affinity instead, as the preceding condition already does. Fixes: sqlalchemy#1120
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.
Fixes the data loss reported in #1120.
cast_for_batch_migrateskips the data transferCASTforJSON, because CASTing toJSONin SQLite yields0. The check wasisinstance(new_type, JSON), which aTypeDecoratorwrappingJSONdoes not satisfy, so theCASTwent out and every value in the column became0.Reproduced on an in-memory SQLite database with one row holding
{"keep": "me", "n": 123}:The change compares type affinity, which the condition on the line above already does:
One note on the approach suggested in the issue.
isinstance(typ.dialect_impl(dialect), JSON)does not catch this case, because for aTypeDecoratordialect_impl()returns the decorator itself rather than its impl. I measured each candidate against the SQLite dialect:isinstancedialect_impl_type_affinity is JSONsa.JSON()TypeDecorator(impl=JSON)sqlite.JSON()postgresql.JSONB()String,Integer,TypeDecorator(impl=String)Affinity is the only one that catches the
TypeDecoratorwhile still excluding non-JSON types.impl_instancealso works, but only for the decorator, so it would have to be combined with the existingisinstancecheck.Worth flagging: this is a behaviour change in one narrow direction.
CASTsuppression now covers any type whose affinity isJSON, not onlyJSONinstances, so a change from a genuinely different affinity to an augmented JSON type no longer emits theCAST. That is what the original guard was for, just applied to the augmented case too.The test is
tests/test_batch.py::CopyFromTest::test_change_type_json_typedecorator, next totest_change_typeas suggested in the issue. It fails on main withCAST(foo.toj AS JSON) AS tojin the transfer statement and passes with the fix. Full suite is 1770 passed, 133 skipped, and there is a changelog fragment indocs/build/unreleased/1120.rst.I used an AI assistant while working on this. The reproduction and the comparison table are things I ran myself.