diff --git a/runtime/metricsview/executor/executor_rewrite_rollup.go b/runtime/metricsview/executor/executor_rewrite_rollup.go index 7f6993549c0..b435d1a2ab6 100644 --- a/runtime/metricsview/executor/executor_rewrite_rollup.go +++ b/runtime/metricsview/executor/executor_rewrite_rollup.go @@ -40,7 +40,6 @@ const ( // Rollup skip reasons: early disqualification const ( skipRawRows = "raw_rows" - skipComparisonTimeRange = "comparison_time_range" skipNonPrimaryTimeDimension = "non_primary_time_dimension" ) @@ -49,21 +48,23 @@ const ( // // Routing decision: // -// 1. Quick disqualification: raw-row queries and comparison time range queries are skipped. +// 1. Quick disqualification: raw-row queries and queries referencing a non-primary time dimension are skipped. // // 2. Eligibility (per rollup): a rollup is eligible only if all of these hold: // a. Query time grain is derivable from the rollup grain (e.g. month from day). // b. For day+ grains, the query timezone matches the rollup timezone. -// c. Query time range start is aligned to the rollup grain. +// c. Both the base and (when present) comparison time range starts are aligned to the rollup grain. // d. All queried dimensions are present in the rollup. -// e. The time range's time dimension is available in the rollup. -// f. All queried measures are present; computed measures (count, count_distinct) are rejected. +// e. The base and comparison time range time dimensions are available in the rollup. +// f. All queried measures are present. Computed measures are rejected except for comparison-derived +// computes (ComparisonValue / ComparisonDelta / ComparisonRatio / ComparisonTime), which are +// allowed as long as their referenced base measure is in the rollup. // g. All WHERE filter dimensions are present in the rollup. // -// 3. Time coverage: an eligible rollup must cover the requested time range. +// 3. Time coverage: an eligible rollup must cover both the base and (when present) comparison time ranges. // For explicit time ranges, the query range is clamped to the base table's actual data range first. // For no-time-range queries ("all data"), the rollup must cover the base table's full min/max range. -// Additionally, if the base table has data beyond the query end, the query end must be aligned +// Additionally, if the base table has data beyond a range's end, that end must be aligned // to the rollup grain (to prevent the last bucket from pulling in extra data). // // 4. Selection: among eligible rollups, prefer the coarsest grain (fewer rows to scan). @@ -100,18 +101,6 @@ func (e *Executor) rewriteQueryForRollup(ctx context.Context, qry *metricsview.Q return nil, nil } - // Disqualify: queries with comparison time ranges - if qry.ComparisonTimeRange != nil { - _, span := tracer.Start(ctx, "rollup.selection") - span.SetAttributes( - attribute.Int("rollup.candidate_count", len(e.metricsView.Rollups)), - attribute.String("rollup.result", "skipped"), - attribute.String("rollup.skip_reason", skipComparisonTimeRange), - ) - span.End() - return nil, nil - } - // Disqualify: queries using a non-primary time dimension (rollups are built on the primary) if qry.TimeRange != nil && qry.TimeRange.TimeDimension != "" && qry.TimeRange.TimeDimension != e.metricsView.TimeDimension { _, span := tracer.Start(ctx, "rollup.selection") @@ -130,9 +119,10 @@ func (e *Executor) rewriteQueryForRollup(ctx context.Context, qry *metricsview.Q // Extract dimension names from the WHERE clause whereDims := collectWhereDimensions(qry.Where) - // Determine whether the query has a non-zero time range using start and end to make sure they are resolved. At this point - // e.RewriteQueryTimeRanges is called earlier, and thus other fields would be unset, and only start and end will be set. + // Determine whether the query has a non-zero time range / comparison time range using start and end to make sure they are resolved. + // At this point e.RewriteQueryTimeRanges is called earlier, and thus other fields would be unset, and only start and end will be set. hasTimeRange := qry.TimeRange != nil && (!qry.TimeRange.Start.IsZero() || !qry.TimeRange.End.IsZero()) + hasComparisonTimeRange := qry.ComparisonTimeRange != nil && (!qry.ComparisonTimeRange.Start.IsZero() || !qry.ComparisonTimeRange.End.IsZero()) // Parent span for rollup selection selectionCtx, selectionSpan := tracer.Start(ctx, "rollup.selection") @@ -216,32 +206,13 @@ func (e *Executor) rewriteQueryForRollup(ctx context.Context, qry *metricsview.Q } rollupEffEnd := timeutil.OffsetTime(rollupMax, timeutil.TimeGrainFromAPI(rollup.TimeGrain), 1, rollupLoc) + // Check coverage for the base time range (or full base table range when no time range is set), + // and additionally for the comparison time range when present. + rangeRejected := false if hasTimeRange { - // Clamp query range to the base table's actual data range. - // This ensures a rollup isn't rejected when the query extends beyond both the base table and rollup. - effectiveStart := qry.TimeRange.Start - if !effectiveStart.IsZero() && baseMin.After(effectiveStart) { - effectiveStart = baseMin - } - effectiveEnd := qry.TimeRange.End - if !effectiveEnd.IsZero() && baseMax.Before(effectiveEnd) { - effectiveEnd = baseMax - } - - // Check coverage: rollup must cover the effective (clamped) range - if !effectiveStart.IsZero() && rollupMin.After(effectiveStart) { - rejectCandidate(rejectStartNotCovered, - attribute.String("rollup.effective_start", effectiveStart.Format(time.RFC3339)), - attribute.String("rollup.rollup_min", rollupMin.Format(time.RFC3339)), - ) - continue - } - if !effectiveEnd.IsZero() && rollupEffEnd.Before(effectiveEnd) { - rejectCandidate(rejectEndNotCovered, - attribute.String("rollup.effective_end", effectiveEnd.Format(time.RFC3339)), - attribute.String("rollup.rollup_eff_end", rollupEffEnd.Format(time.RFC3339)), - ) - continue + if reason, attrs := checkRangeCoverage(qry.TimeRange, baseMin, baseMax, rollupMin, rollupEffEnd); reason != "" { + rejectCandidate(reason, attrs...) + rangeRejected = true } } else { // No time range: rollup must cover the base table's full range @@ -250,28 +221,39 @@ func (e *Executor) rewriteQueryForRollup(ctx context.Context, qry *metricsview.Q attribute.String("rollup.base_min", baseMin.Format(time.RFC3339)), attribute.String("rollup.rollup_min", rollupMin.Format(time.RFC3339)), ) - continue - } - if rollupEffEnd.Before(baseMax) { + rangeRejected = true + } else if rollupEffEnd.Before(baseMax) { rejectCandidate(rejectEndNotCovered, attribute.String("rollup.base_max", baseMax.Format(time.RFC3339)), attribute.String("rollup.rollup_eff_end", rollupEffEnd.Format(time.RFC3339)), ) - continue + rangeRejected = true + } + } + if !rangeRejected && hasComparisonTimeRange { + if reason, attrs := checkRangeCoverage(qry.ComparisonTimeRange, baseMin, baseMax, rollupMin, rollupEffEnd); reason != "" { + rejectCandidate(reason, attrs...) + rangeRejected = true } } + if rangeRejected { + continue + } // Check end alignment now: if data extends beyond the query end and the end is not aligned to the rollup grain, // the last rollup bucket would include data beyond the requested range. rollupEligible only checks start alignment. - // Essentially it just check if base has data >= query end time, then makes sure the query end time is rollup grain aligned - if hasTimeRange && !qry.TimeRange.End.IsZero() && !baseMax.Before(qry.TimeRange.End) && - !timeAligned(qry.TimeRange.End, rollup.TimeGrain, rollupLoc, e.metricsView.FirstDayOfWeek) { - rejectCandidate(rejectEndNotAligned, - attribute.String("rollup.query_end", qry.TimeRange.End.Format(time.RFC3339)), - attribute.String("rollup.base_max", baseMax.Format(time.RFC3339)), - attribute.String("rollup.rollup_grain", rollup.TimeGrain.String()), - ) - continue + // Applies to both the base and comparison time ranges. + if hasTimeRange { + if reason, attrs := checkEndAlignment(qry.TimeRange.End, baseMax, rollup.TimeGrain, rollupLoc, e.metricsView.FirstDayOfWeek); reason != "" { + rejectCandidate(reason, attrs...) + continue + } + } + if hasComparisonTimeRange { + if reason, attrs := checkEndAlignment(qry.ComparisonTimeRange.End, baseMax, rollup.TimeGrain, rollupLoc, e.metricsView.FirstDayOfWeek); reason != "" { + rejectCandidate(reason, attrs...) + continue + } } candidateSpan.SetAttributes(attribute.String("rollup.eligible", "true")) @@ -332,22 +314,27 @@ func rollupEligible(rollup *runtimev1.MetricsViewSpec_Rollup, qry *metricsview.Q } } - // 3. Start time aligned to rollup grain (use rollup timezone for alignment). - // End alignment is checked conditionally in the coverage phase: only when the base table - // has data beyond the query end (to prevent the last rollup bucket from pulling in extra data). - if qry.TimeRange != nil && !qry.TimeRange.Start.IsZero() { - rollupLoc := time.UTC - if rollup.TimeZone != "" { - loc, err := time.LoadLocation(rollup.TimeZone) - if err != nil { - return false, "", fmt.Errorf("invalid timezone %q for rollup %q: %w", rollup.TimeZone, rollup.Table, err) - } - rollupLoc = loc + // 3. Start time aligned to rollup grain (use rollup timezone for alignment) for both the base + // and comparison ranges. End alignment is checked conditionally in the coverage phase: only when + // the base table has data beyond the query end (to prevent the last rollup bucket from pulling in extra data). + rollupLoc := time.UTC + if rollup.TimeZone != "" { + loc, err := time.LoadLocation(rollup.TimeZone) + if err != nil { + return false, "", fmt.Errorf("invalid timezone %q for rollup %q: %w", rollup.TimeZone, rollup.Table, err) } + rollupLoc = loc + } + if qry.TimeRange != nil && !qry.TimeRange.Start.IsZero() { if !timeAligned(qry.TimeRange.Start, rollup.TimeGrain, rollupLoc, firstDayOfWeek) { return false, rejectStartNotAligned, nil } } + if qry.ComparisonTimeRange != nil && !qry.ComparisonTimeRange.Start.IsZero() { + if !timeAligned(qry.ComparisonTimeRange.Start, rollup.TimeGrain, rollupLoc, firstDayOfWeek) { + return false, rejectStartNotAligned, nil + } + } // 4. All query dimensions present in rollup rollupDims := make(map[string]bool, len(rollup.Dimensions)) @@ -386,14 +373,26 @@ func rollupEligible(rollup *runtimev1.MetricsViewSpec_Rollup, qry *metricsview.Q } // 6. All queried measures present in rollup; reject computed measures (count, count_distinct, etc.) - // since they produce incorrect results on pre-aggregated rollup tables. + // since they produce incorrect results on pre-aggregated rollup tables. Comparison-derived + // computed measures (ComparisonValue / ComparisonDelta / ComparisonRatio / ComparisonTime) are + // allowed because they are pure SQL wrappers over a referenced base measure: the actual + // aggregation is done by the referenced measure, which itself must be in the rollup. rollupMeasures := make(map[string]bool, len(rollup.Measures)) for _, m := range rollup.Measures { rollupMeasures[strings.ToLower(m)] = true } for _, m := range qry.Measures { if m.Compute != nil { - return false, rejectComputedMeasure, nil + refName, ok := comparisonReferencedMeasure(m.Compute) + if !ok { + // Non-comparison compute (count, count_distinct, URI, percent_of_total, etc.) + return false, rejectComputedMeasure, nil + } + // refName == "" for ComparisonTime: pure date arithmetic, no measure dependency. + if refName != "" && !rollupMeasures[strings.ToLower(refName)] { + return false, rejectMeasureMissing, nil + } + continue } if !rollupMeasures[strings.ToLower(m.Name)] { return false, rejectMeasureMissing, nil @@ -482,6 +481,69 @@ func collectWhereDimensionsRec(expr *metricsview.Expression, dims map[string]boo } } +// checkRangeCoverage verifies that the rollup covers the given time range. The query range is first +// clamped to the base table's actual data range so a rollup isn't rejected when the query extends +// beyond the base table. Returns the reject reason and trace attributes, or "" if covered. +func checkRangeCoverage(tr *metricsview.TimeRange, baseMin, baseMax, rollupMin, rollupEffEnd time.Time) (string, []attribute.KeyValue) { + effectiveStart := tr.Start + if !effectiveStart.IsZero() && baseMin.After(effectiveStart) { + effectiveStart = baseMin + } + effectiveEnd := tr.End + if !effectiveEnd.IsZero() && baseMax.Before(effectiveEnd) { + effectiveEnd = baseMax + } + if !effectiveStart.IsZero() && rollupMin.After(effectiveStart) { + return rejectStartNotCovered, []attribute.KeyValue{ + attribute.String("rollup.effective_start", effectiveStart.Format(time.RFC3339)), + attribute.String("rollup.rollup_min", rollupMin.Format(time.RFC3339)), + } + } + if !effectiveEnd.IsZero() && rollupEffEnd.Before(effectiveEnd) { + return rejectEndNotCovered, []attribute.KeyValue{ + attribute.String("rollup.effective_end", effectiveEnd.Format(time.RFC3339)), + attribute.String("rollup.rollup_eff_end", rollupEffEnd.Format(time.RFC3339)), + } + } + return "", nil +} + +// checkEndAlignment enforces grain alignment on a range's end when the base table has data at or +// beyond that end (otherwise the last rollup bucket would pull in data past the requested range). +// Returns the reject reason and trace attributes, or "" if no enforcement is needed or it passes. +func checkEndAlignment(end, baseMax time.Time, grain runtimev1.TimeGrain, loc *time.Location, firstDayOfWeek uint32) (string, []attribute.KeyValue) { + if end.IsZero() || baseMax.Before(end) { + return "", nil + } + if timeAligned(end, grain, loc, firstDayOfWeek) { + return "", nil + } + return rejectEndNotAligned, []attribute.KeyValue{ + attribute.String("rollup.query_end", end.Format(time.RFC3339)), + attribute.String("rollup.base_max", baseMax.Format(time.RFC3339)), + attribute.String("rollup.rollup_grain", grain.String()), + } +} + +// comparisonReferencedMeasure returns the base measure referenced by a comparison-derived compute, +// along with ok=true if the compute is a comparison type that's safe to evaluate against a rollup. +// Returns ("", true) for ComparisonTime since it is pure date arithmetic with no measure dependency. +// Returns ("", false) for non-comparison computes (count, count_distinct, percent_of_total, uri), +// which cannot be correctly re-aggregated from a pre-aggregated rollup table. +func comparisonReferencedMeasure(c *metricsview.MeasureCompute) (string, bool) { + switch { + case c.ComparisonValue != nil: + return c.ComparisonValue.Measure, true + case c.ComparisonDelta != nil: + return c.ComparisonDelta.Measure, true + case c.ComparisonRatio != nil: + return c.ComparisonRatio.Measure, true + case c.ComparisonTime != nil: + return "", true + } + return "", false +} + // normalizeTimezone validates and normalizes a timezone string for comparison. // It normalizes UTC variants (empty, "UTC", "Etc/UTC") to "UTC". // Note: Go's time.LoadLocation preserves the input name, so aliases like "US/Eastern" diff --git a/runtime/metricsview/executor/executor_rewrite_rollup_test.go b/runtime/metricsview/executor/executor_rewrite_rollup_test.go index 5491f449652..57f231aba8e 100644 --- a/runtime/metricsview/executor/executor_rewrite_rollup_test.go +++ b/runtime/metricsview/executor/executor_rewrite_rollup_test.go @@ -222,7 +222,161 @@ func TestRewriteQueryForRollup_WhereDimensionMissing(t *testing.T) { require.Nil(t, result) } -func TestRewriteQueryForRollup_ComparisonTimeRange(t *testing.T) { +func TestRollupEligible_ComparisonTimeRange_Aligned(t *testing.T) { + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "total_impressions"}}, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.True(t, eligible, "expected eligible, got reject reason: %s", reason) +} + +func TestRollupEligible_ComparisonTimeRange_StartNotAligned(t *testing.T) { + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "total_impressions"}}, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC), // mid-day, not aligned to day grain + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.False(t, eligible) + require.Equal(t, rejectStartNotAligned, reason) +} + +func TestRollupEligible_ComparisonTimeRange_NonPrimaryTimeDim(t *testing.T) { + // Non-primary time dim on the comparison range is handled at the early-skip layer, + // not rollupEligible. Confirm rollupEligible itself rejects via rejectTimeDimensionMissing + // when the comparison range references a time dim that's not in the rollup. + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "total_impressions"}}, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + TimeDimension: "other_ts", // not in rollup + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.False(t, eligible) + require.Equal(t, rejectTimeDimensionMissing, reason) +} + +func TestRollupEligible_ComparisonValueMeasure(t *testing.T) { + // ComparisonValue references a base measure; if that base measure is in the rollup, it's allowed. + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{ + {Name: "total_impressions"}, + { + Name: "prev_impressions", + Compute: &metricsview.MeasureCompute{ + ComparisonValue: &metricsview.MeasureComputeComparisonValue{Measure: "total_impressions"}, + }, + }, + }, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.True(t, eligible, "expected eligible, got reject reason: %s", reason) +} + +func TestRollupEligible_ComparisonDeltaMissingReferencedMeasure(t *testing.T) { + // ComparisonDelta referencing a measure that's not in the rollup must be rejected. + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, // missing total_clicks + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{ + { + Name: "clicks_delta", + Compute: &metricsview.MeasureCompute{ + ComparisonDelta: &metricsview.MeasureComputeComparisonDelta{Measure: "total_clicks"}, + }, + }, + }, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.False(t, eligible) + require.Equal(t, rejectMeasureMissing, reason) +} + +func TestRollupEligible_CountStillRejected(t *testing.T) { + // Non-comparison computed measures (count, count_distinct, percent_of_total, uri) remain rejected. + rollup := &runtimev1.MetricsViewSpec_Rollup{ + Table: "daily_rollup", + TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, + Measures: []string{"total_impressions"}, + } + qry := &metricsview.Query{ + Measures: []metricsview.Measure{ + {Name: "__count__", Compute: &metricsview.MeasureCompute{Count: true}}, + }, + } + eligible, reason, err := rollupEligible(rollup, qry, runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED, nil, "timestamp", 0) + require.NoError(t, err) + require.False(t, eligible) + require.Equal(t, rejectComputedMeasure, reason) +} + +func TestRewriteQueryForRollup_ComparisonTimeRange_NonPrimaryTimeDim(t *testing.T) { + // A comparison range referencing a non-primary time dimension must skip rollup routing + // at the early-skip layer (without needing to fetch timestamps). e := &Executor{ metricsView: &runtimev1.MetricsViewSpec{ Table: "base_table", @@ -231,25 +385,18 @@ func TestRewriteQueryForRollup_ComparisonTimeRange(t *testing.T) { {Name: "total_impressions", Expression: `SUM("impressions")`}, }, Rollups: []*runtimev1.MetricsViewSpec_Rollup{ - { - Table: "daily_rollup", - TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, - Measures: []string{"total_impressions"}, - }, + {Table: "daily_rollup", TimeGrain: runtimev1.TimeGrain_TIME_GRAIN_DAY, Measures: []string{"total_impressions"}}, }, }, } - qry := &metricsview.Query{ - Measures: []metricsview.Measure{ - {Name: "total_impressions"}, - }, + Measures: []metricsview.Measure{{Name: "total_impressions"}}, ComparisonTimeRange: &metricsview.TimeRange{ - Start: time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC), - End: time.Date(2023, 2, 1, 0, 0, 0, 0, time.UTC), + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + TimeDimension: "other_ts", }, } - result, err := e.rewriteQueryForRollup(context.Background(), qry) require.NoError(t, err) require.Nil(t, result) diff --git a/runtime/metricsview/executor/executor_rollup_integration_test.go b/runtime/metricsview/executor/executor_rollup_integration_test.go index 8333b7a3765..dfc6817ffe4 100644 --- a/runtime/metricsview/executor/executor_rollup_integration_test.go +++ b/runtime/metricsview/executor/executor_rollup_integration_test.go @@ -844,6 +844,160 @@ explore: require.Equal(t, float64(7440), rows[2].impressions, "March") }) + t.Run("comparison_query_uses_rollup", func(t *testing.T) { + // A comparison query with both ranges grain-aligned to a rollup should route to it. + e := newRollupTestExecutor(t, rt, instanceID) + defer e.Close() + + qry := &metricsview.Query{ + Dimensions: []metricsview.Dimension{ + {Name: "timestamp", Compute: &metricsview.DimensionCompute{TimeFloor: &metricsview.DimensionComputeTimeFloor{Dimension: "timestamp", Grain: metricsview.TimeGrainMonth}}}, + }, + Measures: []metricsview.Measure{ + {Name: "total_impressions"}, + { + Name: "prev_impressions", + Compute: &metricsview.MeasureCompute{ + ComparisonValue: &metricsview.MeasureComputeComparisonValue{Measure: "total_impressions"}, + }, + }, + { + Name: "impressions_delta", + Compute: &metricsview.MeasureCompute{ + ComparisonDelta: &metricsview.MeasureComputeComparisonDelta{Measure: "total_impressions"}, + }, + }, + }, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + table := queryAndGetTable(t, e, qry) + // Monthly is coarsest eligible for both ranges + require.Equal(t, rollupTestMonthTable, table) + }) + + t.Run("comparison_misaligned_falls_to_base", func(t *testing.T) { + // Comparison range start is mid-day (not aligned to any day+ grain); all rollups rejected. + e := newRollupTestExecutor(t, rt, instanceID) + defer e.Close() + + qry := &metricsview.Query{ + Measures: []metricsview.Measure{ + {Name: "total_impressions"}, + { + Name: "prev_impressions", + Compute: &metricsview.MeasureCompute{ + ComparisonValue: &metricsview.MeasureComputeComparisonValue{Measure: "total_impressions"}, + }, + }, + }, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC), // mid-day + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + table := queryAndGetTable(t, e, qry) + require.Equal(t, rollupTestBaseTable, table) + }) + + t.Run("comparison_not_covered_falls_to_base", func(t *testing.T) { + // Weekly rollup only has Jan+Feb; comparison range is in Mar — uncovered, weekly rejected. + // Daily and monthly do cover March, so monthly is selected. + e := newRollupTestExecutor(t, rt, instanceID) + defer e.Close() + + qry := &metricsview.Query{ + Dimensions: []metricsview.Dimension{ + {Name: "timestamp", Compute: &metricsview.DimensionCompute{TimeFloor: &metricsview.DimensionComputeTimeFloor{Dimension: "timestamp", Grain: metricsview.TimeGrainMonth}}}, + }, + Measures: []metricsview.Measure{ + {Name: "total_impressions"}, + { + Name: "prev_impressions", + Compute: &metricsview.MeasureCompute{ + ComparisonValue: &metricsview.MeasureComputeComparisonValue{Measure: "total_impressions"}, + }, + }, + }, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 4, 1, 0, 0, 0, 0, time.UTC), + }, + } + table := queryAndGetTable(t, e, qry) + require.Equal(t, rollupTestMonthTable, table) + }) + + t.Run("monthly_comparison_matches_base", func(t *testing.T) { + // Compare Feb (rollup) vs Jan (rollup) at month grain: deltas and ratios must match + // a direct query against the base table. + e := newRollupTestExecutor(t, rt, instanceID) + defer e.Close() + ctx := context.Background() + + qry := &metricsview.Query{ + Dimensions: []metricsview.Dimension{ + {Name: "timestamp", Compute: &metricsview.DimensionCompute{TimeFloor: &metricsview.DimensionComputeTimeFloor{Dimension: "timestamp", Grain: metricsview.TimeGrainMonth}}}, + }, + Measures: []metricsview.Measure{ + {Name: "total_impressions"}, + { + Name: "prev_impressions", + Compute: &metricsview.MeasureCompute{ + ComparisonValue: &metricsview.MeasureComputeComparisonValue{Measure: "total_impressions"}, + }, + }, + { + Name: "impressions_delta", + Compute: &metricsview.MeasureCompute{ + ComparisonDelta: &metricsview.MeasureComputeComparisonDelta{Measure: "total_impressions"}, + }, + }, + }, + Sort: []metricsview.Sort{{Name: "timestamp"}}, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 3, 1, 0, 0, 0, 0, time.UTC), + }, + ComparisonTimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + } + + res, err := e.Query(ctx, qry, nil) + require.NoError(t, err) + defer res.Close() + + // Verify the rollup was used, then read the row. + require.Equal(t, rollupTestMonthTable, e.LatestQueryTable()) + + require.True(t, res.Next()) + var ts time.Time + var base, prev, delta float64 + require.NoError(t, res.Scan(&ts, &base, &prev, &delta)) + require.False(t, res.Next()) + + // Jan: 744 hours * 10 = 7440 (prev); Feb: 696 hours * 10 = 6960 (base, 2024 leap year) + require.Equal(t, float64(6960), base, "Feb base impressions") + require.Equal(t, float64(7440), prev, "Jan prev impressions") + require.Equal(t, float64(-480), delta, "Feb - Jan delta") + }) + t.Run("no_grain_with_filter_correctness", func(t *testing.T) { e := newRollupTestExecutor(t, rt, instanceID) defer e.Close()