feat: Simplify rill time syntax #6879
Conversation
AdityaHegde
force-pushed
the
fix/rill-time-sytax-updates
branch
from
e870785 to
1cb8f66
Compare
| {"rill-MTD", "2024-08-01T00:00:00Z", "2024-08-06T06:32:37Z"}, | ||
| {"rill-PW", "2024-07-29T00:00:00Z", "2024-08-05T00:00:00Z"}, | ||
| {"m", "2025-03-10T06:31:00Z", "2025-03-10T06:32:00Z", timeutil.TimeGrainSecond}, | ||
| {"m~", "2025-03-10T06:32:00Z", "2025-03-10T06:32:37Z", timeutil.TimeGrainSecond}, |
There was a problem hiding this comment.
I can see both sides to this, but my thinking is that the API should always return intervals that start and end on the colloquial boundaries. So, m~ should actually end at 2025-03-10T06:33:00Z even though that is "in the future". The reason for this is threefold 1) whether or not we should display the blank/null data that is in the future is more of a rendering concern and something we want to leave up to the user. 2) When the watermark is in the past, it feels odd to somewhat arbitrarily cut off a complete month. If I ask for M~ and my data is from 6 years ago, it's probably helpful to see the full month even if the data ends halfway through. 3) When making comparisons to previous periods, it's again a determination of the user as to whether they want to compare partial period to full period or partial to partial. When ~ always returns "complete" periods, we don't need to make an unnecessary fetch request for, let's say, M~ to +1M or whatever it might be. The total and time series data would be same, so we can use the query cache for the primary data.
There was a problem hiding this comment.
Hmmm wont this will prevent users from comparing say week-to-date and the same period one week ago? What range will the user have to provide in the new syntax to achieve this?
There was a problem hiding this comment.
It's Thursday today. W~ would give me March 10th to March 17th and seven total data points. The first three (Mo, Tu, We) would be full days, the fourth (Th) would be partial data for today and the 5th, 6th and 7th (Fr, Sa, Su) would be null.
If I wanted no null data points, I would use WTD~ (include Th) or WTD (don't include Th)
When comparing to previous week with, for instance, W~ of -1W or just -1W, I would get seven data points as well, so I'd have a symmetric comparison to the current week. The time series graph would have two lines. One that extends the full interval and one that extends part way (the current period). WTD~ of -1W would be the other option.
In the UI, when comparisons are off, we may give a way to visually truncate those nulls (extend the partial line to the full width of the timeseries graph), but this is a rendering concern and would not be a new API request. When comparisons are on, you need your API request to match the visual truncation or your totals would not be symmetric.
| {"W1 of -2M", "2024-12-30T00:00:00Z", "2025-01-06T00:00:00Z", timeutil.TimeGrainDay}, | ||
| {"D3 of W1 of -3Y", "2022-01-05T00:00:00Z", "2022-01-06T00:00:00Z", timeutil.TimeGrainHour}, | ||
| {"W2 of M11 of +3Y", "2028-11-06T00:00:00Z", "2028-11-13T00:00:00Z", timeutil.TimeGrainDay}, | ||
| {"<3m of H2 of -6D of -1M", "2025-02-04T01:00:00Z", "2025-02-04T01:03:00Z", timeutil.TimeGrainSecond}, |
There was a problem hiding this comment.
I think the rule to follow in general with grains is that if you ask for a single unit of time (like W1, <m, -1H), the default grain should be the next lower order grain, as you have been doing. However, if you ask for values greater than a single unit, as in the case of <3m, the default grain should match that first token. So, I would expect this to default to minutes.
| {"W1 to W3 by W", "2025-03-03T00:00:00Z", "2025-03-24T00:00:00Z", timeutil.TimeGrainWeek}, | ||
| {"W1 of -2M to D", "2024-12-30T00:00:00Z", "2025-03-10T00:00:00Z", timeutil.TimeGrainDay}, | ||
| {"W1 of -2M to D~", "2024-12-30T00:00:00Z", "2025-03-10T06:32:37Z", timeutil.TimeGrainDay}, | ||
| {"-4D to -2D", "2025-03-06T00:00:00Z", "2025-03-09T00:00:00Z", timeutil.TimeGrainHour}, |
There was a problem hiding this comment.
So, my thinking here is that this should be a two day period and not a three day period. I put a poll up on slack, though, because others may not agree. I think people will naturally want the sort of quick mental math to match. If I ask for -12M to -8M, I would want that to return me a 4 month period (12 minus 8). The idea is {start} to {start}. -4D to -2D~ would be a way to get three days.
proto/rill/runtime/v1/queries.proto
Outdated
| // Optional timezone param to easily override time-range expressions | ||
| optional string time_zone = 5; |
There was a problem hiding this comment.
Consider removing optional and just handling the empty value case (protobufs recommend only using optional when there's a material difference between empty and undefined values).
| @@ -87,17 +87,15 @@ func (e *Executor) resolveTimeRange(ctx context.Context, tr *TimeRange, tz *time | |||
| return err | |||
| } | |||
|
|
|||
| tr.Start, tr.End, err = rillTime.Eval(rilltime.EvalOptions{ | |||
| // TODO: use grain when we have timeseries from metrics_view_aggregation | |||
There was a problem hiding this comment.
What do you mean by this?
There was a problem hiding this comment.
rillTime.Eval now returns grain. So once we use have timeseries queries moved over to using we will consume it.
AdityaHegde
changed the title
The current syntax for rill time is not the most ergonomic. We are doing a second pass on the syntax: https://www.notion.so/rilldata/Rill-Time-Documentation-1b2ba33c8f57804fa147cff3cba30121
First(<) andLast(>) modifiers.Next(+) modifier.tojoiner.PeriodToGrain.Checklist: