compare-toplist no dim values fix (#5087)

* compare-toplist no dim values fix * compare-toplist no dim values fix --------- Co-authored-by: Egor Ryashin <[email protected]>
rilldata · Jun 17, 2024 · 0bd59a4 · 0bd59a4 · github-actions · Jun 17, 2024
1 parent c745b9e
commit 0bd59a4
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 8 deletions.
diff --git a/runtime/pkg/expressionpb/expressionpb.go b/runtime/pkg/expressionpb/expressionpb.go
@@ -52,6 +52,17 @@ func In(col *runtimev1.Expression, values []*runtimev1.Expression) *runtimev1.Ex
 	}
 }
 
+func IdentIn(ident string, values ...*runtimev1.Expression) *runtimev1.Expression {
+	return &runtimev1.Expression{
+		Expression: &runtimev1.Expression_Cond{
+			Cond: &runtimev1.Condition{
+				Op:    runtimev1.Operation_OPERATION_IN,
+				Exprs: append([]*runtimev1.Expression{Identifier(ident)}, values...),
+			},
+		},
+	}
+}
+
 func Eq(col, value string) *runtimev1.Expression {
 	return &runtimev1.Expression{
 		Expression: &runtimev1.Expression_Cond{

diff --git a/runtime/queries/metricsview_comparison_toplist.go b/runtime/queries/metricsview_comparison_toplist.go
@@ -205,14 +205,16 @@ func (q *MetricsViewComparison) removeNoSortMeasures() []*runtimev1.MetricsViewA
 }
 
 func (q *MetricsViewComparison) addDimsAsFilter() {
-	inExpressions := make([]*runtimev1.Expression, 0, len(q.Result.Rows))
-	for _, r := range q.Result.Rows {
-		inExpressions = append(inExpressions, expressionpb.Value(r.DimensionValue))
-	}
-	if q.Where != nil {
-		q.Where = expressionpb.And([]*runtimev1.Expression{q.Where, expressionpb.In(expressionpb.Identifier(q.DimensionName), inExpressions)})
-	} else {
-		q.Where = expressionpb.In(expressionpb.Identifier(q.DimensionName), inExpressions)
+	if len(q.Result.Rows) > 0 {
+		inExpressions := make([]*runtimev1.Expression, 0, len(q.Result.Rows))
+		for _, r := range q.Result.Rows {
+			inExpressions = append(inExpressions, expressionpb.Value(r.DimensionValue))
+		}
+		if q.Where != nil {
+			q.Where = expressionpb.And([]*runtimev1.Expression{q.Where, expressionpb.In(expressionpb.Identifier(q.DimensionName), inExpressions)})
+		} else {
+			q.Where = expressionpb.In(expressionpb.Identifier(q.DimensionName), inExpressions)
+		}
 	}
 }
 

diff --git a/runtime/queries/metricsview_comparison_toplist_test.go b/runtime/queries/metricsview_comparison_toplist_test.go
@@ -4,11 +4,14 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"os"
 	"strings"
 	"testing"
+	"time"
 
 	runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1"
 	"github.com/rilldata/rill/runtime"
+	"github.com/rilldata/rill/runtime/pkg/expressionpb"
 	"github.com/rilldata/rill/runtime/queries"
 	"github.com/rilldata/rill/runtime/testruntime"
 	"github.com/stretchr/testify/require"
@@ -640,6 +643,87 @@ func TestServer_MetricsViewTimeseries_export_csv(t *testing.T) {
 	require.Equal(t, "Domain Label,Total volume", rowStrings[0])
 }
 
+func TestMetricsViewsComparison_Druid_comparsion_no_dim_values(t *testing.T) {
+	if os.Getenv("LOCALDRUID") == "" {
+		t.Skip("skipping the test in non-local Druid environment")
+	}
+
+	rt, instanceID := testruntime.NewInstanceForDruidProject(t)
+
+	q := &queries.MetricsViewComparison{
+		MetricsViewName: "ad_bids_metrics",
+		DimensionName:   "dom",
+		Measures: []*runtimev1.MetricsViewAggregationMeasure{
+			{
+				Name: "m1",
+			},
+		},
+		ComparisonMeasures: []string{"m1"},
+		TimeRange: &runtimev1.TimeRange{
+			Start: timestamppb.New(time.Date(2022, 1, 1, 0, 0, 0, 0, time.UTC)),
+			End:   timestamppb.New(time.Date(2022, 1, 2, 0, 0, 0, 0, time.UTC)),
+		},
+		ComparisonTimeRange: &runtimev1.TimeRange{
+			Start: timestamppb.New(time.Date(2022, 1, 2, 0, 0, 0, 0, time.UTC)),
+			End:   timestamppb.New(time.Date(2022, 1, 3, 0, 0, 0, 0, time.UTC)),
+		},
+		Sort: []*runtimev1.MetricsViewComparisonSort{
+			{
+				Name:     "m1",
+				SortType: runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_BASE_VALUE,
+				Desc:     true,
+			},
+		},
+		Where: expressionpb.AndAll(
+			expressionpb.IdentIn("pub", expressionpb.String("Yahoo")),
+			expressionpb.IdentIn("id", expressionpb.Number(0)),
+		),
+		Limit: 250,
+	}
+
+	err := q.Resolve(context.Background(), rt, instanceID, 0)
+	require.NoError(t, err)
+	require.Empty(t, q.Result)
+}
+
+func TestMetricsViewsComparison_comparsion_no_dim_values(t *testing.T) {
+	rt, instanceID := testruntime.NewInstanceForProject(t, "ad_bids")
+
+	q := &queries.MetricsViewComparison{
+		MetricsViewName: "ad_bids_metrics",
+		DimensionName:   "dom",
+		Measures: []*runtimev1.MetricsViewAggregationMeasure{
+			{
+				Name: "m1",
+			},
+		},
+		ComparisonMeasures: []string{"m1"},
+		TimeRange: &runtimev1.TimeRange{
+			Start: timestamppb.New(time.Date(2022, 1, 1, 0, 0, 0, 0, time.UTC)),
+			End:   timestamppb.New(time.Date(2022, 1, 2, 0, 0, 0, 0, time.UTC)),
+		},
+		ComparisonTimeRange: &runtimev1.TimeRange{
+			Start: timestamppb.New(time.Date(2022, 1, 2, 0, 0, 0, 0, time.UTC)),
+			End:   timestamppb.New(time.Date(2022, 1, 3, 0, 0, 0, 0, time.UTC)),
+		},
+		Sort: []*runtimev1.MetricsViewComparisonSort{
+			{
+				Name:     "m1",
+				SortType: runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_BASE_VALUE,
+				Desc:     true,
+			},
+		},
+		Where: expressionpb.AndAll(
+			expressionpb.IdentIn("pub", expressionpb.String("Yahoo1")),
+		),
+		Limit: 250,
+	}
+
+	err := q.Resolve(context.Background(), rt, instanceID, 0)
+	require.NoError(t, err)
+	require.Empty(t, q.Result)
+}
+
 func TestMetricsViewsComparison_comparsion_having_same_name(t *testing.T) {
 	rt, instanceID := testruntime.NewInstanceForProject(t, "ad_bids")
 

diff --git a/runtime/testruntime/testdata/ad_bids_druid/dashboards/ad_bids_metrics.yaml b/runtime/testruntime/testdata/ad_bids_druid/dashboards/ad_bids_metrics.yaml
@@ -19,6 +19,10 @@ dimensions:
   - label: Space Label
     name: space_label
     expression: "publisher"
+  - label: id 
+    name: id
+    expression: "id"
+
 
 measures:
   - label: "Number of bids"