blaze / odo

Data Migration for the Blaze Project
http://odo.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
1.01k stars 138 forks source link

BUG: Remove mssql.TIMESTAMP #568

Closed thequackdaddy closed 7 years ago

thequackdaddy commented 7 years ago

xref #567 and https://github.com/blaze/blaze/pull/1656

cc @postelrich

dhirschfeld commented 7 years ago

It seems to me that it's a bug in sqlalchemy? The mssql module imports the TIMESTAMP type but according to the docs it isn't the same type as the ANSI SQL TIMESTAMP.

i.e. isinstance(sa.TIMESTAMP(), mssql.TIMESTAMP) should fail

Manually patching my sqlalchemy:

--- lib/sqlalchemy/dialects/mssql/base.py
+++ lib/sqlalchemy/dialects/mssql/base.py
@@ -617,13 +617,18 @@ from ... import engine
 from ...engine import reflection, default
 from ... import types as sqltypes
 from ...types import INTEGER, BIGINT, SMALLINT, DECIMAL, NUMERIC, \
-    FLOAT, TIMESTAMP, DATETIME, DATE, BINARY,\
+    FLOAT, DATETIME, DATE, BINARY,\
     TEXT, VARCHAR, NVARCHAR, CHAR, NCHAR

 from ...util import update_wrapper
 from . import information_schema as ischema

+
+# The mssql TIMESTAMP value is *not* the ANSI SQL TIMESTAMP type
+# https://msdn.microsoft.com/en-us/library/ms182776%28v=SQL.90%29.aspx
+TIMESTAMP = BINARY
+
 # http://sqlserverbuilds.blogspot.com/
 MS_2016_VERSION = (13,)
 MS_2014_VERSION = (12,)
-- 

...seems to fix the odo problem for me:

In [1]: from sqlalchemy.dialects import mssql
   ...: import sqlalchemy as sa
   ...: from odo import discover
   ...: columns = [sa.Column('amount', sa.REAL),
   ...:            sa.Column('ds', sa.TIMESTAMP)]
   ...: dta = sa.Table('t', sa.MetaData(), *columns)
   ...: discover(dta)
Out[1]: dshape("var * {amount: ?float32, ds: ?datetime}")

In [2]: isinstance(sa.TIMESTAMP(), mssql.TIMESTAMP)
Out[2]: False

...and doesn't seem to break sqlalchemy (same number of tests failed for me before and after the patch)

dhirschfeld commented 7 years ago

xref: BitBucket#4092, BitBucket#4086

postelrich commented 7 years ago

Closing as #589 implemented my suggestion.