feat: Add new rill time parser backend support #6326

AdityaHegde · 2024-12-23T06:21:55Z

Add module for parsing rill time syntax in backend
Support syntax like, 2024-12-15+5d,2024-12-15-5d
Add ResolveTime to return start and end times based on minTime, maxTime and now for a metrics view
Replace usage of old duration parser
Map old format to new format
Add API to resolve times for a list of rill-times
Add a lot more tests
~~Update MetricsViewTimeSeries to use TimeRange and make use of ResolveTime~~ We will just use the MetricsViewAggregation in a future PR. If it is not ready then this can be added.

Note 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.

runtime/compilers/rillv1/parse_explore.go

runtime/compilers/rillv1/parse_metrics_view.go

runtime/metricsview/executor_rewrite_time.go

runtime/pkg/metricssql/time_range_parser.go

runtime/pkg/metricssql/parser.go

proto/rill/runtime/v1/queries.proto

begelundmuller · 2025-01-09T15:04:57Z

runtime/compilers/rillv1/parse_explore.go

+				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 != "" {


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

Update. Had to move ValidateISO8601 to achieve this.

begelundmuller · 2025-01-09T15:16:41Z

runtime/metricsview/executor_rewrite_time.go

 	"github.com/rilldata/rill/runtime/pkg/timeutil"
 )

+func (e *Executor) GetMinTime(ctx context.Context, colName string) (time.Time, error) {


Can drop the Get, so just MinTime, similar to Watermark.

All the executors exported functions should be in runtime/metricsview/executor.go file – this probably belongs next to the Watermark function.

Why does this accept a column name? We currently only support one time dimension for metrics views.

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/metricsview/executor_rewrite_time.go

proto/rill/runtime/v1/queries.proto

begelundmuller · 2025-01-09T15:50:46Z

runtime/server/queries_metrics.go

+	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(),


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.

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.

begelundmuller · 2025-01-09T15:53:01Z

runtime/pkg/rilltime/rilltime_test.go

+		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"},


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"},

Wouldnt it be 2024-08-09T10:32:00Z to 2024-08-09T10:32:36Z?

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):

runtime/pkg/rilltime/rilltime.go

begelundmuller · 2025-01-14T16:54:25Z

Awaiting integration of this PR before the next review: #6413

…time-syntax-backend

begelundmuller · 2025-01-17T16:13:41Z

runtime/queries/metricsview_resolve_time_ranges.go

+	"google.golang.org/protobuf/types/known/timestamppb"
+)
+
+type MetricsViewTimeRanges struct {


This basically just a) calls the timestamps resolver (which may be cached in the resolver cache), b) calls rilltime, which doesn't involve heavy operations and should be fast. So can the logic here just be moved directly to the API handler for MetricsViewTimeRanges? Since there is no need for separate caching of the result here.

It's also nice not to add new logic to the runtime/queries package since it's being deprecated.

begelundmuller · 2025-01-17T16:19:28Z

runtime/pkg/rilltime/rilltime.go

+
+	if rt.End == nil {
+		rt.End = &TimeAnchor{
+			Now: true,


Shouldn't the watermark be the default end if not specified?

Lemme check with mike today. The docs mention that it should be now. But it probably make sense to do latest/watermark.

begelundmuller · 2025-01-17T16:20:34Z

runtime/pkg/rilltime/rilltime.go

+		rt.isNewFormat = true
+	}
+
+	rt.timeZone = time.UTC


Can we add support for passing in the default time zone? E.g. we have had some requests to use the same time zone as the refresh: clause when evaluating and formatting data for alerts and reports.

begelundmuller · 2025-01-17T16:29:36Z

runtime/pkg/rilltime/rilltime_test.go

+		{`m : |s|`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:37Z"},
+		{`m : s`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:37Z"},


Shouldn't it be:

{`m : |s|`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:36Z"},

like it was before? I.e. |s| leads to :36Z and s leads to :37Z.

The idea being that |s| indicates that only completed bins should be included – but if maxTime=...:36Z, more events may yet appear with time=...:36Z. And callers who are good with bins that have potentially partial data should use m : s.

Hmmm. So the idea is that our bins are ts >= 0 and ts < next 0? So a data point at the boundary is actually part of next grain. That is why |s| will not include it since it is incomplete. We should make sure we explain all these things in our docs, will note it down.

Add rilltime package

a55bf2d

AdityaHegde marked this pull request as draft December 23, 2024 06:22

AdityaHegde added 8 commits December 23, 2024 14:30

Use rilltime in queries

2aa5ca3

Add API to resolve time ranges

3f165ed

Add absolute date/time support

3bbb615

Adding basic +/- support to anchors

a947e7f

Adding mapping for old rill-<> format

6d51ab9

Merge branch 'main' into adityahegde/rill-time-syntax-backend

6162f08

Merge branch 'main' into adityahegde/rill-time-syntax-backend

da5e3fb

Support 2024-03-01-7d, 2024-03-01 @-2d

34c287f

AdityaHegde requested a review from begelundmuller January 7, 2025 06:03

Merge branch 'main' into adityahegde/rill-time-syntax-backend

9f89121

AdityaHegde marked this pull request as ready for review January 7, 2025 06:05

AdityaHegde requested a review from k-anshul January 7, 2025 06:05

Fix merge issue

7cf5de0

k-anshul requested changes Jan 8, 2025

View reviewed changes

AdityaHegde added 2 commits January 8, 2025 17:27

Review comments #1

330aabe

Remove unit param in metricssql time functions

3b5dd34

k-anshul requested changes Jan 9, 2025

View reviewed changes

runtime/pkg/metricssql/time_range_parser.go Outdated Show resolved Hide resolved

runtime/pkg/metricssql/parser.go Outdated Show resolved Hide resolved

runtime/pkg/metricssql/parser.go Outdated Show resolved Hide resolved

AdityaHegde added 2 commits January 9, 2025 16:25

Review comments pass 2

9233683

Merge branch 'main' into adityahegde/rill-time-syntax-backend

ca73407

begelundmuller requested changes Jan 9, 2025

View reviewed changes

More PR comments

e9029c2

AdityaHegde force-pushed the adityahegde/rill-time-syntax-backend branch from b9d86fa to e9029c2 Compare January 10, 2025 11:15

AdityaHegde added 4 commits January 10, 2025 18:08

Add watermark to metricsview_time_range resolver

786408e

Fix ISO duration and some edge cases

c0f8eb4

Merge branch 'main' into adityahegde/rill-time-syntax-backend

1fbb43a

Merge branch 'main' into feat/watermark-calc-to-resolver

306689f

AdityaHegde requested review from k-anshul and begelundmuller January 14, 2025 05:25

AdityaHegde force-pushed the adityahegde/rill-time-syntax-backend branch from 3b6a03d to b3e69ab Compare January 14, 2025 05:27

Add watermark as separate param

43b7da2

AdityaHegde force-pushed the adityahegde/rill-time-syntax-backend branch from b3e69ab to 43b7da2 Compare January 14, 2025 05:34

AdityaHegde added 3 commits January 14, 2025 18:00

Move timestamp calc to executor

011dd45

Add a test around usage of ISO ranges

c70143f

Use cached timestamps in metrics aggreagtion query

0cc9827

AdityaHegde added 11 commits January 15, 2025 12:01

PR comments

313aea8

Add to resolver_test

691a9c5

Cleanup column_time_range

896490e

Fix tests

b21361c

Merge branch 'feat/watermark-calc-to-resolver' into adityahegde/rill-…

ba8ec27

…time-syntax-backend

Cache timestamps in metrics resolver

76026c5

Handle metrics view without timestamp

3c0061a

Merge branch 'feat/watermark-calc-to-resolver' into adityahegde/rill-…

4df8d71

…time-syntax-backend

Integrate metrics_time_range resolver

6078cff

Add test for time_range_start/end functions

3254b83

Merge branch 'main' into adityahegde/rill-time-syntax-backend

96a571b

begelundmuller reviewed Jan 17, 2025

View reviewed changes

begelundmuller requested changes Jan 17, 2025

View reviewed changes

Default anchors to watermark instead of now

f480b54

AdityaHegde force-pushed the adityahegde/rill-time-syntax-backend branch from 8d9f34a to f480b54 Compare January 20, 2025 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add new rill time parser backend support #6326

feat: Add new rill time parser backend support #6326

AdityaHegde commented Dec 23, 2024 •

edited

Loading

begelundmuller Jan 9, 2025

AdityaHegde Jan 13, 2025

begelundmuller Jan 9, 2025

AdityaHegde Jan 13, 2025

begelundmuller Jan 9, 2025

AdityaHegde Jan 14, 2025 •

edited

Loading

begelundmuller Jan 9, 2025

AdityaHegde Jan 10, 2025

begelundmuller Jan 10, 2025

begelundmuller commented Jan 14, 2025 •

edited

Loading

begelundmuller Jan 17, 2025

begelundmuller Jan 17, 2025

AdityaHegde Jan 20, 2025

begelundmuller Jan 17, 2025

begelundmuller Jan 17, 2025

AdityaHegde Jan 20, 2025

		{`m : \|s\|`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:37Z"},
		{`m : s`, "2024-08-09T10:32:00Z", "2024-08-09T10:32:37Z"},

feat: Add new rill time parser backend support #6326

Are you sure you want to change the base?

feat: Add new rill time parser backend support #6326

Conversation

AdityaHegde commented Dec 23, 2024 • 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

AdityaHegde Jan 14, 2025 • 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

begelundmuller commented Jan 14, 2025 • 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

AdityaHegde commented Dec 23, 2024 •

edited

Loading

AdityaHegde Jan 14, 2025 •

edited

Loading

begelundmuller commented Jan 14, 2025 •

edited

Loading