Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add presto dttm format #522

Closed
wants to merge 1 commit into from

Conversation

tarekrached
Copy link

the default dttm string was giving me GREATER_THAN_OR_EQUAL(timestamp, varchar) not registered errors with presto, this resolves that.

@coveralls
Copy link

coveralls commented May 26, 2016

Coverage Status

Coverage remained the same at 82.165% when pulling 7284e1f on tarekrached:presto-dttm-converter into 7d27692 on airbnb:master.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 7284e1f on tarekrached:presto-dttm-converter into 7d27692 on airbnb:master.

@wgzhao
Copy link

wgzhao commented May 27, 2016

great work!

it just i want to need.

@mistercrunch
Copy link
Member

Oh interesting, we use presto and the current auto-cast works for us. What version of presto are you using?

@tarekrached
Copy link
Author

this is for 0.130, an earlier amazon sandbox release...

@mistercrunch
Copy link
Member

Since Hive does not support a native datetime data type, a lot of our Presto timestamp columns are just ISO text, so this "fix" would break pretty much all of our slices.

The solution is probably in that other PR where you can define your date function on a per-table (but I'm asking the author to move to per-column) basis. Closing, sorry.

@tarekrached
Copy link
Author

Right on, we'll monkey patch until we upgrade presto. Thanks for checking this out!

@tarekrached
Copy link
Author

tarekrached commented Jun 16, 2016

hey @mistercrunch we're running into this again. Does your comment above mean that all your presto timestamp comparisons are being done as lexical string comparisons? I don't think presto actually auto-casts varchars to timestamps (or at least I couldn't find anything in any of the release notes from 0.130 to 0.147)), so this means that if we do have timestamp columns, we would need to cast them to strings.

I appreciate the bind you're in due to storing ISO 8601 strings as dates, but shouldn't we use db-native timestamp comparisons, as in the oracle conversion directly before this change. Seems inconsistent to do string comparisons in some DBs and timestamp comparisons in others, especially when timestamps are available in presto.

Which PR were you referring to that has the per-table date functions?

@mistercrunch
Copy link
Member

Right right. I think that PR was closed. We probably need something like this (per column casting function definition) as database can have DATE, DATETIME, TIMESTAMP and people often store dates as strings or epoch.

You're right about Presto, though it's odd as most people use Hive to load up the data that ultimately is made available in Presto, and Hive doesn't have date/time data types. The situation that Airbnb is in probably the most common one.

@tarekrached
Copy link
Author

tarekrached commented Jun 16, 2016

I think #506 (Dttm format) might be the PR you were talking about, but wouldn't that still be comparing timestamps lexically? Totally agree that should be on a per-column basis tho. Maybe I'm confused about Hive - didn't it get TIMESTAMP in 2011?

This feels like the wrong venue for this discussion - I'll open a new issue in a bit.

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants