feat: Add new rill time parser backend support #6326
Conversation
| rt, err := rilltime.Parse(ctr.Range) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid comparison range %q: %w", ctr.Range, err) | ||
| } | ||
| isNewFormat = rt.IsNewFormat | ||
| } | ||
| if ctr.Offset != "" { |
There was a problem hiding this comment.
Consider having a rilltime.ParseCompatibility(ctr.Range, ctr.Offset) function – this would avoid duplicating the error handling if isoOffset is combined with the new format, and also avoid exposing the IsNewFormat outside the package
There was a problem hiding this comment.
Update. Had to move ValidateISO8601 to achieve this.
| "github.com/rilldata/rill/runtime/pkg/timeutil" | ||
| ) | ||
|
|
||
| func (e *Executor) GetMinTime(ctx context.Context, colName string) (time.Time, error) { |
There was a problem hiding this comment.
- Can drop the
Get, so justMinTime, similar toWatermark. - All the executors exported functions should be in
runtime/metricsview/executor.gofile – this probably belongs next to theWatermarkfunction. - Why does this accept a column name? We currently only support one time dimension for metrics views.
There was a problem hiding this comment.
Update for 1 and 2. For 3, we are currently reusing the method for time_range_start and time_range_end which accepts a column name.
runtime/server/queries_metrics.go
Outdated
| timeRangeQuery := &queries.MetricsViewTimeRange{ | ||
| MetricsViewName: req.MetricsViewName, | ||
| MetricsView: mv.ValidSpec, | ||
| ResolvedMVSecurity: security, | ||
| } | ||
| err = s.runtime.Query(ctx, req.InstanceId, timeRangeQuery, int(req.Priority)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| q := &queries.MetricsViewResolveTimeRanges{ | ||
| MetricsViewName: req.MetricsViewName, | ||
| MinTime: timeRangeQuery.Result.TimeRangeSummary.Min.AsTime(), |
There was a problem hiding this comment.
It's weird that it resolves MinTime separately here, but it resolves the watermark/max time inside the MetricsViewResolveTimeRanges implementation.
I would expect a) that it generally resolves these timestamps the same way in most places, b) that it resolves these timestamps at the same place / subject to the same caching.
There was a problem hiding this comment.
The idea was to utilise the cache for calculating min and max. Will raise a follow up to use the metrics_time_range resolver which also have the watermark.
| end string | ||
| }{ | ||
| // Earliest = 2023-08-09T10:32:36Z, Latest = 2024-08-06T06:32:36Z, = Now = 2024-08-09T10:32:36Z | ||
| {`m : |s|`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:36Z"}, |
There was a problem hiding this comment.
In the (resolved) comments on the doc, we discussed open intervals that end on a time boundary, but I don't think there's a test case for that here. Can you add it? I think an example could be:
{`m : s`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:37Z"},
There was a problem hiding this comment.
Wouldnt it be 2024-08-09T10:32:00Z to 2024-08-09T10:32:36Z?
There was a problem hiding this comment.
I believe it should be ...:37Z – there was a long comment thread about it in the design doc, which seems to have been resolved without updating the main document. Here's one of the relevant comments (to find it, change the comment filter to include resolved comments in Notion):
AdityaHegde
force-pushed
the
adityahegde/rill-time-syntax-backend
branch
from
b9d86fa to
e9029c2
Compare
AdityaHegde
force-pushed
the
adityahegde/rill-time-syntax-backend
branch
from
3b6a03d to
b3e69ab
Compare
AdityaHegde
force-pushed
the
adityahegde/rill-time-syntax-backend
branch
from
b3e69ab to
43b7da2
Compare
|
Awaiting integration of this PR before the next review: #6413 |
2024-12-15+5d,2024-12-15-5dResolveTimeto return start and end times based on minTime, maxTime and now for a metrics viewUpdateWe will just use the MetricsViewAggregation in a future PR. If it is not ready then this can be added.MetricsViewTimeSeriesto useTimeRangeand make use ofResolveTimeNote that this doesnt add support for rill-time in alerts and report intervals (The underlying query is support in this PR). That will be added in a future PR.