Druid filters in configured timezone

[moumny]

Related to #3386 .
Filters in druid (intervals) are now made in the report timezone configured instead of UTC.

[salsakran]

Awesome.

Could you add some tests that show the bug so we can make sure there are no regressions in the future?

[camsaul]

This is definitely going to break things, there's lots of places in the code where we user this function and they're not passing the extra timezone arg

[camsaul]

@moumny, the changes you made in metabase.util will definitely break some of our other drivers. date-trunc is used all over the place. Changing the arity of functions like that will break all of the other code that calls those functions.

[camsaul]

The current iteration of date-trunc is a 1 or 2 arity function, so when you change it to 2 or 3 all the places that were calling the 1-arity version will throw an ArityException and all the places that call the 2-arity version will be passing the wrong arguments.

If you need to pass a third argument then a better solution would be to add a third arity of the function

[moumny]

Sorry should have put WIP in the title.
The functions are only used in druid and mongo driver.
I decided to update mongo driver to make it explicit that the timezone is UTC.

We should probably add some test in timeseries testsuite that take into account the report timezone and test the timestamp filters. Would need bit of help for that ^^. (especially that mongo for example filters still in UTC i guess)

Still todo: verifiy that i didnt break mongo driver

[moumny]

✅ mongodb tests are passing :)

[camsaul]

@moumny this is looking good but as I mentioned above

If you need to pass a third argument then a better solution would be to add a third arity of the function

i.e. if "UTC" is basically the default timezone-id argument for things like relative-date and date-trunc I'd rather keep the existing arities and have those pass "UTC" as a default arg, and then add a new arity that takes timezone-id. For example, I'd prefer date-extract look like this after your changes:

(defn date-extract
  ([unit]
   (date-extract unit (System/currentTimeMillis)))
  ([unit date]
   (date-extract unit date "UTC"))
  ([unit date timezone-id])
   ...))

That way the Mongo driver wouldn't have to be changed at all. It looks like you did this with
->Calendar already

[camsaul]

is this being used?

[camsaul]

https://github.com/bbatsov/clojure-style-guide#sugared-java-interop

[camsaul]

Actually you should probably just do (->Timestamp trunced-ms)

[camsaul]

👍 on the extra tests

[moumny]

Thanks for your comment @camsaul !
I changed the arity to be consistent with the rest. The idea was to avoid defaults to UTC to avoid mistakes. Its convenient to call one of the first UTC methods in druid driver and its harder to spot than (date-op date "UTC") :)

Anyway let me know if you see something else. 👁

[camsaul]

@moumny, this looks good 👍 . I'll run it on CI in a bit and if all the tests pass then we can merge this

Thanks for your contribution 😍

[camsaul]

@moumny I had to make a couple last-minute tweaks to this so we can ship it as part of our 0.20 release. I opened a new PR for it, #3459, so I'm going to go ahead and close this one out.

I'll merge your fixes in as soon as the tests pass on CI. Thanks again for your contribution! 🎉