[PLAT-440] roll up routing in metrics view

[pjain1]

• Add rollup table config to metrics view proto and YAML parser
• Implement query routing: eligible rollups are selected based on grain derivability, dimension/measure coverage, timezone match, time range alignment, and time coverage
• Prefer coarsest grain among eligible rollups; break ties by smallest data range
• For no-time-range queries ("all data"), verify the rollup covers the base table's full range rather than skipping coverage checks

Checklist:

• Covered by tests
• Ran it and it works as intended
• Reviewed the diff before requesting a review
• Checked for unhandled edge cases
• Linked the issues it closes
• Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• Intend to cherry-pick into the release branch
• I'm proud of this work!

[begelundmuller]

What is the reasoning for this default? I'm wondering if it would be safer to make it required. Worried about three problems:

• If it is misconfigured, you will get broken/empty results (fails silently instead of with a clear error)
• If not explicitly set, the smallest_time_grain defaults to seconds by default, which is unlikely to be used for rollups
• smallest_time_grain is supposed to indicate the granularity of the raw table; but won't the rollup tables usually have a different granularity than the raw table?
• We can add this default later if needed, but we cannot easily make it required later. So might be safer to make it required now.

[pjain1]

Its required and does not default to anything, the comment is out of date, see this in parse_metrics_view.go

  if rollup.TimeGrain == "" {
			return fmt.Errorf(`rollup[%d]: "time_grain" is required`, i)
		}

will fix the comment.

[begelundmuller]

The reason we haven't passed the name in before is because executor is supposed to be isolated from the catalog/resolver calls. This has two advantages:

• It keeps logic clean/flattened, avoiding e.g. resolvers calling an executor that calls a resolver
• It enables the executor to safely be used for specs that are not valid in the catalog, which currently the metrics view reconciler uses to validate new specs

I see this is being used to call a resolver for time ranges. I understand that in practice it works since it avoids circular code paths, but given how complex this package is, I think we should try to stick to the above principles about the package being isolated to avoid circular code.

We had a similar problem earlier for effectively resolving time expressions. At that time, we introduced the BindQuery function, which solved the problem in a clean way:

• If the query is not bound, the executor directly fetches the timestamps (which ensures correctness, but doesn't have any caching)
• The caller can optionally bind the query using cached timestamps to speed things up. See an example here – it works out quite cleanly in practice:

  rill/runtime/queries/metricsview_aggregation.go

  Line 143 in 1b982b3

  e, err := executor.New(ctx, rt, instanceID, q.MetricsViewName, mv.ValidSpec, mv.Streaming, security, opts.Priority, userAttrs)

Could we do something similar here? Or maybe just extend BindQuery to bind a full set of timestamps for all the base/rollup tables?

[pjain1]

ok let me have a look, I thought since we are already passing mv spec then passing its name should be fine but I get your point.

[begelundmuller]

There are other fields to check. Consider using qry.TimeRange.IsZero()

[pjain1]

At this point the query time range will be resolved as e.RewriteQueryTimeRanges is called before this and thus other fields will be unset and only start and end will be set. And actually we need it to be resolved as we need query start and end for further checks so this explicitly checks that only and fallbacks to base unless rollup table has same coverage as base.

But if you want I can change to use qry.TimeRange.IsZero() and add a comment that this rewrite should only be called after RewriteQueryTimeRanges or add explicit checks for it.

[begelundmuller]

If an error happens during a span, I think you'd expect it to be added to the span. Otherwise it looks like the error came after the span, which is confusing.

This applies in many of the cases below

[pjain1]

ok will fix.

[begelundmuller]

What if the qry contains a Rill time expression? Would have expected it to resolve the time range to a fixed start/end using the normal code paths for that.

[pjain1]

Yes this rewrite is called after e.RewriteQueryTimeRanges and thus only start and end will be set. See the above related comment.

[begelundmuller]

Shouldn't it also consume the rollup's dimension/measure selectors to filter out the dimensions/measures that are not in the rollup? Otherwise, the synthetic spec contains invalid dimensions/measures that would fail if queried.

[pjain1]

Those things are already checked in rollupEligible method which is called much before this so rollups satisfying the selection criteria (that include dim/measures checks) will be present here and thus keeping it simple when creating this spec. What do you think?

[begelundmuller]

My earlier comment applies to the resolver call here.

I'm not sure, but I was wondering if we should solve this problem by refactoring Executor.Timestamps to query the min/max/watermark timestamps for the base and all the rollup tables in the metrics view, and return them in one go? It would keep things simple and integrate nicely with the existing caching facilitated by BindQuery (externally of the package) and e.timestamps (internally in the package). It would also enable us to return time ranges for different grains to the UI, which in future it may need if we need to optimize rollup usage better.

The downside would be it runs some queries eagerly, which might seem like it could have performance implications, but I wonder if it's okay given a) the output will be cached, b) several of the time ranges are anyway needed for the subsequent rollup checks.

Just an idea, let me know what you think.

[pjain1]

Yeah there were couple of things -

1. If rollup fails basic eligibility test then no need to fetch timestamps for it.
2. Also the executor will have user's security policy so timestamps may not reflect actual boundaries which are needed for further eligibility checks.
3. Directly calling e.timestamps only populates executor level cache which is request specific and cannot be used for subsequent requests, thats why creating a resolver can lead to usage of cached timestamps till cache_timestamps_ttl_seconds.

[begelundmuller]

We already have a metrics_time_range resolver, which sounds almost like the same as this. Would be nice if we could build on that. (Relates to me comment above also.)

[pjain1]

I have put a comment on this resolver as to how its different from metrics_time_range here. Let me paste it here-

// This has two main differences from the metrics_time_range resolver:
//  1. It bypasses row-level security because timestamps reflect physical data boundaries, not user-visible data.
//     Access is restricted to internal callers and admins.
//  2. It implements cache_timestamps_ttl_seconds based caching for efficiency purposes.

So essentially no security based row policies used here as this is used for checking rollup coverage and eligibility and second it adds additional cache that we use to prevent flooding timestamp queries to olap specially for streaming models which has caching disabled by default. Its cache key also relies on metrics_cache_key resolver so it can utilize cache_key_ttl and cache_key_sql but just adds additional logic on top of it to prevent querying based on cache_timestamps_ttl_seconds.

Because of these reasons I created a separate resolver as adding all this logic to metrics_time_range will make the logic complicated. What do you think?