-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Support offset argument in the GROUP BY time(...) call #6504
Conversation
I think this does what we talked about and should make timezone support beyond the normal time parsing code unnecessary. If we want, we can possibly add support later for specifying the offset as a string in the offset column. Any formatting of the output can be handled on the client side. |
Some more work may need to be done though for supporting things like 1 day though. I limited the specified offset to something lower than the duration, but after reading the other issue, I'm not sure that's good enough for solving this ticket completely. |
a7fdb84
to
dbec384
Compare
@jsternberg Overall it lgtm. However, why is there a limitation on the offset to be less than the time duration? You should be able to offset -7h for a time zone but you have 1h intervals. |
@benbjohnson I wasn't sure about that. My reasoning was that the offset is only useful for moving the buckets and moving the bucket more than the interval doesn't make a lot of sense because of modulo math. If you want to limit the time based on a time zone, then you can just use the timezone in the time field like this:
The time when a timezone offset is useful is when you're calculating an aggregate when grouping by day and this works:
This will shift the buckets by 7 hours so that it starts at midnight in the MST time zone. |
For example, Nepal has a UTC+05:45 offset. It would be nice to just put I don't think that limiting the offset gains us anything. |
It's possible we could just do the mod ourselves for user-friendliness. The time positions are still going to be limited by the duration. So you could say |
dbec384
to
09a8585
Compare
I've modified the commit to perform the modulo automatically.
|
👍 |
ae2b5cb
to
4226125
Compare
This needs an InfluxQL docs change. @pauldix can you take a look as well? |
There's also an ongoing discussion about the use of the word |
The discussion of |
I disagree with that. If we support |
An offset of `time(1m, now())` will anchor the offset to the current time of the query. The default offset is `0s` which is the current default anyway. This fixes #2074 by making time zone offset support unnecessary. Time comparisons can use timezones inside of the time clause and the offset needed for non-hour timezone differences can be used as part of the offset argument.
4226125
to
64556e4
Compare
I updated the PR to have |
LGTM 👍 |
Does the right thing happen when my query is half in and half out of DST? I'm going to assume not -- and that should be made clear in the documentation. So if I query Friday March 11 to Monday March 14 (march 13, 2016 dst started) my UTC offset will be (in the eastern time zone) -5 and then at 2am Sunday March 13 it switches to -4. |
This doesn't really assess timezones specifically. All outputs are still in UTC and it's up to the client to determine how to output the value. This is mostly for offsetting the buckets to facilitate buckets that don't fall neatly in non-UTC timezones such as India which is at UTC+05:30. You can do |
An offset of
time(1m, now())
will anchor the offset to the start timeof the query. The default offset is
0s
which is the current defaultanyway.
This fixes #2074 by making time zone offset support unnecessary. Time
comparisons can use timezones inside of the time clause and the offset
needed for non-hour timezone differences can be used as part of the
offset argument.