More PR comments

runtime/compilers/rillv1/parse_metrics_view.go

@@ -234,6 +234,10 @@ func (p *Parser) parseMetricsView(ctx context.Context, node *Node) error {
 			}
 		}
 
+		if (dim.Column == "" && dim.Expression == "") || (dim.Column != "" && dim.Expression != "") {
+			return fmt.Errorf("exactly one of column or expression should be set for dimension: %q", dim.Name)
+		}
+
 		lower := strings.ToLower(dim.Name)
 		if ok := names[lower]; ok {
 			return fmt.Errorf("found duplicate dimension or measure name %q", dim.Name)

runtime/compilers/rillv1/parser_test.go

@@ -89,6 +89,7 @@ materialize: true
 model: m2
 dimensions:
   - name: a
+    column: a
 measures:
   - name: b
     expression: count(*)
@@ -174,7 +175,7 @@ SELECT * FROM {{ ref "m2" }}
 			MetricsViewSpec: &runtimev1.MetricsViewSpec{
 				Table: "m2",
 				Dimensions: []*runtimev1.MetricsViewSpec_DimensionV2{
-					{Name: "a"},
+					{Name: "a", Column: "a"},
 				},
 				Measures: []*runtimev1.MetricsViewSpec_MeasureV2{
 					{Name: "b", Expression: "count(*)"},
@@ -841,6 +842,7 @@ dashboards:
 table: t1
 dimensions:
   - name: a
+    column: a
 measures:
   - name: b
     expression: count(*)
@@ -850,6 +852,7 @@ measures:
 table: t2
 dimensions:
   - name: a
+    column: a
 measures:
   - name: b
     expression: count(*)
@@ -868,7 +871,7 @@ security:
 			MetricsViewSpec: &runtimev1.MetricsViewSpec{
 				Table: "t1",
 				Dimensions: []*runtimev1.MetricsViewSpec_DimensionV2{
-					{Name: "a"},
+					{Name: "a", Column: "a"},
 				},
 				Measures: []*runtimev1.MetricsViewSpec_MeasureV2{
 					{Name: "b", Expression: "count(*)"},
@@ -887,7 +890,7 @@ security:
 			MetricsViewSpec: &runtimev1.MetricsViewSpec{
 				Table: "t2",
 				Dimensions: []*runtimev1.MetricsViewSpec_DimensionV2{
-					{Name: "a"},
+					{Name: "a", Column: "a"},
 				},
 				Measures: []*runtimev1.MetricsViewSpec_MeasureV2{
 					{Name: "b", Expression: "count(*)"},
@@ -971,6 +974,7 @@ func TestMetricsViewAvoidSelfCyclicRef(t *testing.T) {
 model: d1
 dimensions:
   - name: a
+    column: a
 measures:
   - name: b
     expression: count(*)
@@ -985,7 +989,7 @@ measures:
 			MetricsViewSpec: &runtimev1.MetricsViewSpec{
 				Table: "d1",
 				Dimensions: []*runtimev1.MetricsViewSpec_DimensionV2{
-					{Name: "a"},
+					{Name: "a", Column: "a"},
 				},
 				Measures: []*runtimev1.MetricsViewSpec_MeasureV2{
 					{Name: "b", Expression: "count(*)"},

runtime/queries/metricsview.go

@@ -189,7 +189,7 @@ func identifierIsUnnest(mv *runtimev1.MetricsViewSpec, expr *runtimev1.Expressio
 func dimensionSelect(mv *runtimev1.MetricsViewSpec, dim *runtimev1.MetricsViewSpec_DimensionV2, dialect drivers.Dialect) (dimSelect, unnestClause string) {
 	colName := safeName(dim.Name)
 	if !dim.Unnest || dialect == drivers.DialectDruid {
-		return fmt.Sprintf(`%s as %s`, metricsViewDimensionExpression(dim), colName), ""
+		return fmt.Sprintf(`(%s) as %s`, metricsViewDimensionExpression(dim), colName), ""
 	}
 
 	unnestColName := safeName(tempName(fmt.Sprintf("%s_%s_", "unnested", dim.Name)))
@@ -283,20 +283,20 @@ func buildLikeExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Conditio
 	var clause string
 	// Build [NOT] len(list_filter("dim", x -> x ILIKE ?)) > 0
 	if unnest && dialect != drivers.DialectDruid {
-		clause = fmt.Sprintf("%s len(list_filter(%s, x -> x ILIKE %s)) > 0", notKeyword, leftExpr, rightExpr)
+		clause = fmt.Sprintf("%s len(list_filter((%s), x -> x ILIKE %s)) > 0", notKeyword, leftExpr, rightExpr)
 	} else {
 		if dialect == drivers.DialectDruid {
 			// Druid does not support ILIKE
 			clause = fmt.Sprintf("LOWER(%s) %s LIKE LOWER(CAST(%s AS VARCHAR))", leftExpr, notKeyword, rightExpr)
 		} else {
-			clause = fmt.Sprintf("%s %s ILIKE %s", leftExpr, notKeyword, rightExpr)
+			clause = fmt.Sprintf("(%s) %s ILIKE %s", leftExpr, notKeyword, rightExpr)
 		}
 	}
 
 	// When you have "dim NOT ILIKE '...'", then NULL values are always excluded.
 	// We need to explicitly include it.
 	if cond.Op == runtimev1.Operation_OPERATION_NLIKE {
-		clause += fmt.Sprintf(" OR %s IS NULL", leftExpr)
+		clause += fmt.Sprintf(" OR (%s) IS NULL", leftExpr)
 	}
 
 	return clause, args, nil
@@ -347,17 +347,17 @@ func buildInExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition,
 		var clause string
 		// Build [NOT] list_has_any("dim", ARRAY[?, ?, ...])
 		if unnest && dialect != drivers.DialectDruid {
-			clause = fmt.Sprintf("%s list_has_any(%s, ARRAY[%s])", notKeyword, leftExpr, questionMarks)
+			clause = fmt.Sprintf("%s list_has_any((%s), ARRAY[%s])", notKeyword, leftExpr, questionMarks)
 		} else {
-			clause = fmt.Sprintf("%s %s IN (%s)", leftExpr, notKeyword, questionMarks)
+			clause = fmt.Sprintf("(%s) %s IN (%s)", leftExpr, notKeyword, questionMarks)
 		}
 		clauses = append(clauses, clause)
 	}
 
 	if inHasNull {
 		// Add null check
 		// NOTE: DuckDB doesn't handle NULL values in an "IN" expression. They must be checked with a "dim IS [NOT] NULL" clause.
-		clauses = append(clauses, fmt.Sprintf("%s IS %s NULL", leftExpr, notKeyword))
+		clauses = append(clauses, fmt.Sprintf("(%s) IS %s NULL", leftExpr, notKeyword))
 	}
 	var condsClause string
 	if exclude {
@@ -369,7 +369,7 @@ func buildInExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition,
 		// When you have "dim NOT IN (a, b, ...)", then NULL values are always excluded, even if NULL is not in the list.
 		// E.g. this returns zero rows: "select * from (select 1 as a union select null as a) where a not in (1)"
 		// We need to explicitly include it.
-		condsClause += fmt.Sprintf(" OR %s IS NULL", leftExpr)
+		condsClause += fmt.Sprintf(" OR (%s) IS NULL", leftExpr)
 	}
 
 	return condsClause, args, nil

runtime/reconcilers/metrics_view.go

@@ -154,18 +154,15 @@ func (r *MetricsViewReconciler) validate(ctx context.Context, mv *runtimev1.Metr
 }
 
 func validateDimension(ctx context.Context, olap drivers.OLAPStore, t *drivers.Table, d *runtimev1.MetricsViewSpec_DimensionV2, fields map[string]*runtimev1.StructType_Field) error {
-	if d.Expression == "" {
-		if d.Column == "" {
-			return nil
-		}
+	if d.Column != "" {
 		if _, isColumn := fields[d.Column]; !isColumn {
 			return fmt.Errorf("failed to validate dimension %q: column %q not found in table", d.Name, d.Column)
 		}
 		return nil
 	}
 
 	err := olap.Exec(ctx, &drivers.Statement{
-		Query:  fmt.Sprintf("SELECT %s FROM %s GROUP BY 1", d.Expression, safeSQLName(t.Name)),
+		Query:  fmt.Sprintf("SELECT (%s) FROM %s GROUP BY 1", d.Expression, safeSQLName(t.Name)),
 		DryRun: true,
 	})
 	if err != nil {