Incubator-superset: Wrong datetime formating when building the query

[gbrian]

HI,

I've found that datetime is being custom formatted before sending to SQLAlchemy that causes incompatibility between Datetime and Datetime2 in SQLServer.

I'll check to use the SQLAlchemy dialect of the datasource to format instead.

[mistercrunch]

that would really help! I knew this was going to hit on some db with different autocast...

[gbrian]

HI,

Haven't found a nice way to let SQLAlchemy dialect to convert the date time yet.
BTW I'm managing with this patch/workaround at models.py line 542*

            # UGLY: I guess correct way is to delegate on SQLAlchemy dialect
            # UPDATE: Datetime depends on each dialect and I haven't found an easy way to manage
            #         Maybe we can allow user to define its custome format at Database definition
            def get_dtformat(type):
                if type == 'SMALLDATETIME' or type == 'DATETIME':
                    return '%Y-%m-%d %H:%M:%S'
                if type == 'DATE':
                    return '%Y-%m-%d'
                if type == 'TIME':
                    return '%H:%M:%S'
                return '%Y-%m-%d %H:%M:%S.%f'

            tf = get_dtformat(cols[granularity].type or 'DATE')

*Sorry for posting code, I'm facing some issues managing new branches and PR.

[rmnguleria]

Thanks for the workaround .

[pablolemosassis]

I am still finding problem with this issue. I just updated to latest caravel.
Should I do anything to make this #283 fix to work?

[mistercrunch]

283 was not the right approach. we have yet to find a solution to this.

[mistercrunch]

This should help https://github.com/airbnb/caravel/pull/446, please test and report whether it fixes your issues.

[xrmx]

@gbrian is this fixed for you?

[gbrian]

Hi @xrmx,

Sorry not working due to precision:

Don't know the best fix, I'll suggest trim precision.
http://stackoverflow.com/questions/19025192/convert-varchar-to-datetime-in-sql-which-is-having-millisec/38843397#38843397

[gbrian]

@xrmx : If you agree we can use column data type to decide if we can use DateTime2 or trim milliseconds. I mean in case column has been defined as DateTime2 cast to DateTime2, if not just trim (but not TSQL version).
Will this work for you?

[xrmx]

@gbrian adding a proper python_date_format wouldn't fix that? I know it's manual and not working out of the box is lame.

[gbrian]

@xrmx proper way (IMO)

-- Using default and 126 format for Datetime2
SELECT CONVERT(DATETIME2, '2015-08-10 11:58:47.123456', 126) as MSSQLDateTime2
-- Using default[:-3] and 121 format for Datetime 
SELECT CONVERT(DATETIME, '2015-08-10 11:58:47.123', 121) as MSSQLDateTime2

so
some changes in the code like:

def dttm_sql_literal(self, dttm, type):
        """Convert datetime object to string

        If database_expression is empty, the internal dttm
        will be parsed as the string with the pattern that
        the user inputted (python_date_format)
        If database_expression is not empty, the internal dttm
        will be parsed as the sql sentence for the database to convert
        """
        tf = self.python_date_format or '%Y-%m-%d %H:%M:%S.%f'
        if self.database_expression:
            return self.database_expression.format(dttm.strftime('%Y-%m-%d %H:%M:%S'))
        elif tf == 'epoch_s':
            return str((dttm - datetime(1970, 1, 1)).total_seconds())
        elif tf == 'epoch_ms':
            return str((dttm - datetime(1970, 1, 1)).total_seconds() * 1000.0)
        else:
            default = "'{}'".format(dttm.strftime(tf))
            iso = dttm.isoformat()
            d = {
                'mssql': "CONVERT({}, '{}', {})".
                            format("DATETIME2" if type.lower() == "datetime2" else "DATETIME",
                            default if type.lower() == "datetime2" else default[:- 3],
                            126 if type.lower() == "datetime2" else 121),  # untested
                'mysql': default,
                'oracle':
                    """TO_TIMESTAMP('{}', 'YYYY-MM-DD"T"HH24:MI:SS.ff6')""".format(iso),
                'presto': default,
                'sqlite': default,
            }
            for k, v in d.items():
                if self.table.database.sqlalchemy_uri.startswith(k):
                    return v
            return default

I think is this way I keep as much precision as possible with the definition the user has done.

[xrmx]

@gbrian a diff is easier to review :) Anyway the chain of ifs insinde the format is a no-go imho :) Also we can remove the for loop and the dict if we need to patch only mssql and oracle. Also in mssql case do we really need the CONVERT, can't we just return a slice of default instead?

uri = self.table.database.sqlachemy_uri
if uri.startswith('oracle'):
    iso = dttm.isoformat()
   return """TO_TIMESTAMP('{}', 'YYYY-MM-DD"T"HH24:MI:SS.ff6')""".format(iso)
elif uri.startswith('mssql'):
    field_type = type.upper()
    if field_type == 'DATETIME':
        return default[:-3]
    return default
else:
    return default

[gbrian]

Yeah! totally agree with the "ifs" ;) was just playing around, sorry. Sadly I don't have an older SQL Server version to test the "default" behavior so I though CONVERT will be more backward compatible: http://www.techonthenet.com/sql_server/functions/convert.php (basically will cover SQL Server 2005)

Let me try again:

[xrmx]

@gbrian do we really need the convert for the DATETIME2? it looks like it can handle our default just fine.
Anyway open a pull request, looks pretty good anyway and an improvement over current code :)
BTW please reorder the ifs to check for mssql first, i moved there just for c&p convenience :)

[gbrian]

Ah, ok! yep you are right, for DATETIME2, CONVERT is not needed. Creating PR, talk later

[AndreiPashkin]

What is the point of formatting dates within Superset, why don't let SQLAlchemy do that?