Time datatype support #6109

senior · 2017-10-06T13:54:19Z

This PR adds support for Time datatypes on those databases that support them. java.sql.Time objects in Java are a subclass of java.util.Date, but have their year/month/day set to epoch. On some databases, lack of support for Time datatypes will maybe them look like all of the times took place on 1970-01-01. On other databases (like BigQuery) this will cause errors.

This PR adds support for returning Time fields in query results and querying/filtering using time fields.

TODO:

- Add handling for Time objects in SQL databases that support them
- Add handling for Time objects in BigQuery
- Add handling for Time objects in Presto
- Add support for Time query parameters
- UI should use just a "time picker" for time fields

camsaul · 2017-10-20T21:52:30Z

src/metabase/driver/bigquery.clj

@@ -283,7 +302,7 @@
  (let [sql     (str "-- " (qputil/query->remark outer-query) "\n" (if (seq params)
                                                                     (unprepare/unprepare (cons sql params))
                                                                     sql))
-        results (process-native* database sql)
+         results (process-native* database sql)


accidental space?

camsaul · 2017-10-20T22:01:05Z

src/metabase/util.clj

@@ -113,6 +113,22 @@
  java.sql.Timestamp     (->iso-8601-datetime [this timezone-id] (time/unparse (ISO8601Formatter timezone-id) (coerce/from-sql-time this)))
  org.joda.time.DateTime (->iso-8601-datetime [this timezone-id] (time/unparse (ISO8601Formatter timezone-id) this)))

+(def ^:private time-formatter


This is not super important but for functions defined by def it's nice to add :arglists metadata so when you're playing around in the CIDER repl it will show you what args it expects in the minibuffer. e.g.

(def ^:private ^{:arglists '([timezone-id])} time-formatted ...)

Normally I wouldn't even bother to suggest this but since it's in metabase.util it might be worth it to go back and add it

camsaul · 2017-10-20T22:02:31Z

test/metabase/driver/mysql_test.clj

+      {:field-name "number-of-cans", :base-type {:native "tinyint(1)"}}]
+     [["Six Pack"              6]
+      ["Toucan"                2]
+      ["Empty Vending Machine" 0]]]])


what is this fake dataset 😂

We need to add this to the Sample Data 😂

camsaul · 2017-10-20T22:13:53Z

test/metabase/test/data/dataset_definitions.clj

+        .getTime)))
+
+(defn- time-only
+  "This function will return a java.sql.Time object. To create a Time


This isn't important but something I learned about recently that you might find useful:

I've just started using column-enforce-mode in Emacs. It's a minor mode that font locks text past a certain column (defined by column-enforce-column) as red. Maybe worth looking into.

It looks like these lines are all about ~70 characters wide, so I'm guessing you're trying to keep them less than the classic 80 chars, and since they're a little under 80 I'm guessing you're eyeballing them. So maybe it's a tool that you might find useful to make your life a little easier

Do you have a preference on line length? I'm betting what I did here is write a big docstring, then M-q paredit-reindent-defun. I'm usually a fan of 120 chars, but I haven't changed the paredit defaults.

Yeah, 120 is a good number. It seems like GitHub's limit is 120 characters before they add horizontal scrollbars to code, but with the + at the beginning of a PR I've figured out that you can only actually do 119 characters before GH cuts it off. So I have my column limit set to 118 just to be safe.

I didn't know about paredit-reindent-defun, I'll have to check that out. I've just been setting up a keyboard macro to do re-indentation manually like a chump 😓

I think I've found a way to automate this. if you set the vars below:

;; Anywhere (setq clojure-docstring-fill-column 118) ;; In the clojure-mode-hook (set-fill-column 118)

The first will set clojure docstrings correctly at 118 and the second one will set code comments to wrap at 118, then you can just use the built in emacs fill-paragraph stuff that M-q uses.

@sbelak You might be interested in this too.

camsaul · 2017-10-20T22:17:00Z

test/metabase/test/data/presto.clj

@@ -10,7 +13,7 @@
  (:import java.util.Date
           metabase.driver.presto.PrestoDriver))

-(resolve-private-vars metabase.driver.presto execute-presto-query! presto-type->base-type quote-name quote+combine-names)
+(resolve-private-vars metabase.driver.presto execute-presto-query! presto-type->base-type quote-name quote+combine-names time->str)


Going forward it's preferable IMO to stop using resolve-private-vars and just do something like #'presto/time->str when you want to use something private in a test. The only reason I wrote resolve-private-vars in the first place was because I didn't realize you could do that

Also, don't forget to require metabase.driver.presto, otherwise import will potentially barf because Clojure doesn't know it needs to load a certain namespace for that class to get defined. It's working fine here because somebody else is implicitly loading it but relying on such things is sketchy

Those sorts of errors usually end up popping up in confusing ways, like if you try to test a single namespace that ends up skipping loading all the other namespaces that normally implicitly load whatever it is you're expecting to be loaded

Oops. I just noticed you didn't add this code and it was already like that... 😬

Mind fixing it while you're here?

Totally agree on the metabase.driver.presto thing and the resolve-private-vars I have always found confusing as the #' syntax is so easy. I'm 👍, I'll get it fixed up

camsaul

Looks really good!

senior · 2018-01-11T01:08:41Z

@tlrobinson Rebased on master but still fixing up test failures. Pushed up a fix for presto, bigquery is breaking now. I'll work on getting this green first thing in the morning

tlrobinson · 2018-01-15T05:08:49Z

@metabase/core-developers The frontend for this is working, but could use some UI guidance from @mazameli / @kdoh, in particular for the "between" filter:

tlrobinson · 2018-01-15T05:14:34Z

It also occurs to me that a common use-case for filtering by a time is on an extract from a date/time field (e.x. all records created_at 9am to 5pm, where created_at is a date/time). However that generalizes to other extracts like day-of-week, which we don't support in the UI currently anyway, so it's probably best handled together as a separate feature.

camsaul · 2018-01-16T18:11:33Z

For the UI we should probably let you choose AM/PM or 24-hour clock somewhere since the majority of the world uses the 24-hour clock e.g. 17:00 instead of 5:00 PM

mazameli · 2018-01-16T18:37:19Z

@camsaul We wouldn't put an option to choose a 24-hour clock choice in the filter UI itself (Yet Another Knob™), but I definitely think that's something we could and should put as a global setting in the General part of the admin panel. There's a whole host of international formatting options that should really be included there, like default currency symbol, numeric separator style, first day of the week, etc.

mazameli · 2018-01-16T19:00:25Z

@tlrobinson Here's some UI guidance, broken up into the quickest and simplest thing we could do, and then an incremental improvement that we could tackle as a separate project to improve the time selection component globally. Let me know if this is what you were looking for.

Simplest thing

For Between, we'd just include more space between the two time selectors. This is the same exact width as the Between state for Date filters. (Incidentally, a nicety would be to put the focus inside the first input box so you could type right away and tab between inputs.)

For situations where only a single time needs to be chosen, we could put everything on a single line, like so:

Where I'd like to go eventually

I'm including this here for context (and on the off chance that it's trivial work). I think we could improve the time selection component and make it a single input box that would let you type hours, minutes, and am/pm — even allow 24-hour syntax.

I think this is an improvement for a few reasons: saves space and simplifies the UI a bunch; still allows for custom times like 5:17pm; requires fewer clicks and less interaction.

So I could type 5p, and on blur we'd change that to 5:00pm. Or 17:30, and we'd change it to 5:30pm (at least, until we support the 24-hour clock format for real). FYI, I'm totally cribbing from the new Google Calendar here; basically, whatever it lets you input, and the way it interprets that input, is what I'm thinking. (We could also talk about whether we'd want to implement the supplemental dropdowns that GCal has for its version of time input fields; I can see arguments for both sides.)

Examples:

Between

Before

tlrobinson · 2018-01-18T07:40:04Z

@metabase/core-developers I believe this is ready for review

senior · 2018-01-18T14:17:08Z

👍 the backend was reviewed by @camsaul already, so just needs a front end review

attekei · 2018-01-19T14:32:00Z

Could an example time field be added to the sample dataset? That would both make reviewing PRs like this easier and more importantly make it possible to write integration tests for this feature.

After the sample dataset is updated (ping @senior), it's easy for Tom or me to add integration tests which verify that you are able to create a time filter in QB and edit its values afterwards.

Frontend code itself looks good and it seems that visualizations display time values correctly.

Issues I found:

Time field values are shown as Invalid date for SQL questions, not cool
Dashboard filter / SQL parameter filter is missing – if I recall correctly, that's a lot trickier to implement than the QB filter. Should it be in the scope of this PR?
This isn't that important in the context of this PR but as a European I find these AM/PM time pickers super annoying 😃

senior · 2018-01-19T15:32:04Z

@attekei For testing I took the test-data set that the backend uses and split one of the datetime columns to separate date and time fields https://github.com/metabase/metabase/pull/6109/files#diff-0dfb1f2e187020bac353f144592a0137R58. Is something like that usable by you? If not I can look into adding something to the sample datatset, but my guess is it'd need to be a new/separate table with new generated data.

attekei · 2018-01-19T18:42:42Z

@senior Yeah I also created a new column to one of my local PostgreSQL test databases.

I'm referring to a conversation our team had in autumn related to #6266, where we wanted that all x-ray insights should be testable with our sample dataset data. That also made writing frontend tests easy.

If not I can look into adding something to the sample datatset, but my guess is it'd need to be a new/separate table with new generated data.

Yeah if a time column doesn't make sense in the context of any current tables (Orders / People / Products / Reviews), then maybe creating a new table is the only option... but it would still be useful both for the discoverability of the feature and easiness of frontend testing. I don't want to make this a hard blocker but it's still something that would be really nice to have.

mazameli · 2018-01-23T21:15:52Z

What @attekei said. This is going to be a PITA for me to test the UI.

salsakran · 2018-01-24T21:24:52Z

can we add a column to the sample database? Users could be given a "wakeupTime" time column

salsakran · 2018-01-24T22:14:30Z

Filtering looks fine, but the list of breakout granularities looks incorrect -

salsakran

Breakout granularities should change for time fields

salsakran · 2018-01-24T22:21:09Z

oh, and even the ones that are relevant (eg Minute of Hour) don't actually work.

senior · 2018-01-24T22:43:27Z

@salsakran @tlrobinson it shouldn't be showing any bucketing. Databases don't support bucketing of time fields like they do date fields, so any bucketing we'd have to implement ourselves.

tlrobinson · 2018-01-25T16:12:57Z

This reminds me, our Date, Time, and DateTime type hierarchy kind of bother me.

Currently:

Date is a DateTime
Time is a DateTime

But we also use DateTime to mean fields that have both date and time components.

I feel like DateTime should actually be called something like Temporal (TimeLike?), which means it can be rendered like timeseries, but DateTime should only be a type for fields that have Date AND Time. Like this:

Date is a Temporal
Time is a Temporal
DateTime is a Date
DateTime is a Time

As it is, to determine if something has (at least) a time component you have to check that it's a DateTime but not Date, likewise for date component that it's a DateTime but not Time.

tlrobinson · 2018-01-25T16:17:32Z

In relation to this PR, I'm not sure if it's better to change isDate to exclude type/Time and find places where we need to add an || isTime() (or replace with isTemporal or whatever), or to try to find places where an && !isTime() should be added.

tlrobinson · 2018-01-25T16:31:30Z

@senior Actually I forgot, the breakout options are sent by the backend now. Do you mind handling it there?

senior · 2018-01-25T17:08:57Z

@tlrobinson Just pushed up a change so that dimension options aren't included for time fields

senior · 2018-01-25T18:36:45Z

Are we good to go on this? If so I'll squash the commits and force push for merge

This commit adds code needed to query time fields and to filter by time fields. Support is included for all database backends that support time fields. Previously depending on the backend, the result of querying a time field was either an exception or an incorrect "zeroed out" date of 1970-01-01 followed by the time. Fixes #5563, fixes #5829

This primarily focuses on a time picker rather than the date picker when users are attempting to filter based on a time column. There are also some minor changes to how time field results are displayed/formatted.

This was referenced Oct 6, 2017

Update Presto docker container to include the "test-data-with-time" dataset #6110

Closed

Add the test-data-with-time catalog willtrnr/presto-mb-ci#1

Merged

senior force-pushed the time-field-support branch from 83c7bc7 to e30bdc1 Compare October 6, 2017 15:33

senior force-pushed the time-field-support branch 4 times, most recently from cba5a87 to 2948225 Compare October 20, 2017 17:15

camsaul reviewed Oct 20, 2017

View reviewed changes

camsaul approved these changes Oct 20, 2017

View reviewed changes

senior force-pushed the time-field-support branch from 2948225 to 5b0388b Compare October 23, 2017 17:11

senior mentioned this pull request Dec 7, 2017

Big Query TIME type not handled #5826

Closed

senior force-pushed the time-field-support branch 2 times, most recently from fc88f48 to aae113d Compare January 10, 2018 23:58

tlrobinson self-assigned this Jan 15, 2018

tlrobinson requested review from salsakran, mazameli and kdoh January 18, 2018 07:39

salsakran requested changes Jan 24, 2018

View reviewed changes

senior force-pushed the time-field-support branch 3 times, most recently from 49c139b to 283bed9 Compare January 25, 2018 20:41

iethree added 2 commits January 25, 2018 15:51

Add frontend support for time fields

2a8dc4f

This primarily focuses on a time picker rather than the date picker when users are attempting to filter based on a time column. There are also some minor changes to how time field results are displayed/formatted.

senior force-pushed the time-field-support branch from 283bed9 to 2a8dc4f Compare January 25, 2018 21:56

senior merged commit 3aad62d into master Jan 25, 2018

senior deleted the time-field-support branch January 25, 2018 22:40

salsakran added this to the 0.28 milestone Jan 25, 2018

senior mentioned this pull request Jun 25, 2018

Add Special Type "Time" #5319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time datatype support #6109

Time datatype support #6109

senior commented Oct 6, 2017 •

edited by tlrobinson

Loading

camsaul Oct 20, 2017

camsaul Oct 20, 2017

camsaul Oct 20, 2017

camsaul Oct 20, 2017 •

edited

Loading

senior Oct 23, 2017

camsaul Oct 23, 2017

senior Oct 23, 2017

camsaul Oct 20, 2017

camsaul Oct 20, 2017

camsaul Oct 20, 2017

senior Oct 23, 2017

camsaul left a comment

senior commented Jan 11, 2018

tlrobinson commented Jan 15, 2018 •

edited

Loading

tlrobinson commented Jan 15, 2018

camsaul commented Jan 16, 2018

mazameli commented Jan 16, 2018

mazameli commented Jan 16, 2018

tlrobinson commented Jan 18, 2018

senior commented Jan 18, 2018

attekei commented Jan 19, 2018 •

edited

Loading

senior commented Jan 19, 2018

attekei commented Jan 19, 2018

mazameli commented Jan 23, 2018

salsakran commented Jan 24, 2018

salsakran commented Jan 24, 2018

salsakran left a comment

salsakran commented Jan 24, 2018

senior commented Jan 24, 2018

tlrobinson commented Jan 25, 2018

tlrobinson commented Jan 25, 2018

tlrobinson commented Jan 25, 2018

senior commented Jan 25, 2018

senior commented Jan 25, 2018

Time datatype support #6109

Time datatype support #6109

Conversation

senior commented Oct 6, 2017 • edited by tlrobinson Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsaul Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsaul left a comment

Choose a reason for hiding this comment

senior commented Jan 11, 2018

tlrobinson commented Jan 15, 2018 • edited Loading

tlrobinson commented Jan 15, 2018

camsaul commented Jan 16, 2018

mazameli commented Jan 16, 2018

mazameli commented Jan 16, 2018

Simplest thing

Where I'd like to go eventually

tlrobinson commented Jan 18, 2018

senior commented Jan 18, 2018

attekei commented Jan 19, 2018 • edited Loading

senior commented Jan 19, 2018

attekei commented Jan 19, 2018

mazameli commented Jan 23, 2018

salsakran commented Jan 24, 2018

salsakran commented Jan 24, 2018

salsakran left a comment

Choose a reason for hiding this comment

salsakran commented Jan 24, 2018

senior commented Jan 24, 2018

tlrobinson commented Jan 25, 2018

tlrobinson commented Jan 25, 2018

tlrobinson commented Jan 25, 2018

senior commented Jan 25, 2018

senior commented Jan 25, 2018

senior commented Oct 6, 2017 •

edited by tlrobinson

Loading

camsaul Oct 20, 2017 •

edited

Loading

tlrobinson commented Jan 15, 2018 •

edited

Loading

attekei commented Jan 19, 2018 •

edited

Loading