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

Already on GitHub? Sign in to your account

SQLite dialect: Support reflection for columns with data affinities #65

Merged
merged 3 commits into from Feb 16, 2014

Conversation

Projects
None yet
2 participants
Contributor

eblume commented Feb 4, 2014

Quick note: included in this PR are two commits that I made while learning the code base. The first is a simple ignore for "_.egg" and the second is a rather sweaping PEP8 formatting change for the sqlite dialect base.py. _Please feel free to ask me to remove these.* Some people find things like that trivial and pedantic - no sweat, I can rebase them out of existence and resubmit the PR, no problem.

Otherwise, included is reflection support for SQLite databases that have bizarre column names. This is a feature of SQLite, and without it reflection is broken for some valid SQLite databases.

I had to add an @Skip('sqlite') to one of the generic reflection tests to get all of the tests to pass. This test was a test to ensure that the database emits a warning when reflection failed to determine the type of a column. Because the affinity algorithm will always find a valid data type for a column, no such situation exits any more. Therefore skipping the test for sqlite seemed the right thing to do... let me know if you disagree.

Thanks, and please let me know if I should include anything else or change my approach.

eblume added some commits Jan 31, 2014

SQLite dialect - support relection from affinity
SQLite allows column types that aren't technically understood in sqlite
by using 'data affinity', which is an algorithm for converting column
types in to some sort of useful type that can be stored and retrieved
from the db. Unfortunatly, this breaks reflection since we (previously)
expected a sqlite db to reflect column types that we permit in the
`ischema_names` for that dialect.

This patch changes the logic for 'unknown' column types during
reflection to instead run through SQLite's data affinity algorithm, and
assigns appropriate types from that.

It also expands the matching for column type to include column types
with spaces (strongly discouraged but allowed by sqlite) and also
completely empty column types (in which case the NullType is assigned,
which sqlite will treat as a Blob - or rather, Blob is treated as
NullType). These changes mean that SQLite will never raise an error for
an unknown type during reflection - there will always be some 'useful'
type returned, which follows the spirit of SQLite (accomodation before
sanity!).
Owner

zzzeek commented Feb 4, 2014

Ok this is great but how i want it to work is that the column reflection consults ischema_names first, then drops down into the type affinity resolution. I know that ideally this isn't that important, but there's a lot of folks that really want their specific types to be taking effect and there may even be some who are populating ischema_names specifically. The pep8 thing is great too but i want to make sure that's a separate commit.

Contributor

eblume commented Feb 4, 2014

The affinity function does in fact consult ischema_names first and uses that even if the affinity rules would match differently - do you mean that you want me to pull that ischema_names check out of the affinity function? If so, will do! And yes, the pep8 stuff is entirely in a separate commit.

Glad you like it! At the risk of sounding vain, does this contribution count for being added to the contributors list? :)

Owner

zzzeek commented Feb 4, 2014

oh, so it does, thought that one test skip was for that reason. OK. So for pullreq I pull in your commit, I credit you in the changelog, and i think you can even add yourself to http://www.sqlalchemy.org/trac/wiki/Contributors (or i will!). Then there's AUTHORS, which is like, seven people. If you want to be in there, great, if you hit 30 or 40 commits it'll be no problem :)

Contributor

eblume commented Feb 4, 2014

I'll keep my eyes peeled for things to fix and subscribe to the mailing list then. :) If you wouldn't mind adding me (Erich Blume) to the contributors page, that'd be swell - one must register to do that and I try to keep my internet registrations to a minimum. Of course if I do stick around and aim for AUTHOR status I'd be happy to register, hah!

Contributor

eblume commented Feb 4, 2014

And to clarify, the skip is because that test expects the db to raise an Unknown Type warning during reflection, but SQLite no longer has a concept of an 'unknown type'.

bug here, the trailing comma turns this into a tuple. the test fails too. fixing now

Owner

eblume replied Feb 16, 2014

Hah! Good catch.

Owner

eblume replied Feb 16, 2014

Hmmm I guess my linter isn't working. For shame. Thanks!

@zzzeek zzzeek merged commit e47f994 into zzzeek:master Feb 16, 2014

Owner

zzzeek commented Feb 16, 2014

alrighty, I also reworked all the reflection tests in SQLite into a new thing that's organized. the existing tests there were extremely old. see https://github.com/zzzeek/sqlalchemy/blob/master/test/dialect/test_sqlite.py#L953.

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