Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dashboard] Support arbitrary non-aggregate expressions in dimensions #3702

Merged
merged 15 commits into from
Dec 22, 2023
Merged
14 changes: 12 additions & 2 deletions proto/gen/rill/runtime/v1/resources.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions proto/gen/rill/runtime/v1/resources.pb.validate.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions proto/gen/rill/runtime/v1/runtime.swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2424,6 +2424,8 @@ definitions:
type: string
column:
type: string
expression:
type: string
label:
type: string
description:
Expand Down
1 change: 1 addition & 0 deletions proto/rill/runtime/v1/resources.proto
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ message MetricsViewSpec {
message DimensionV2 {
string name = 1;
string column = 2;
string expression = 6;
string label = 3;
string description = 4;
bool unnest = 5;
Expand Down
18 changes: 7 additions & 11 deletions runtime/compilers/rillv1/parse_metrics_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type MetricsViewYAML struct {
Name string
Label string
Column string
Expression string
Comment on lines 36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we don't have validation to ensure that only one of these are set (exactly one of them should be non-nil)

Property string // For backwards compatibility
Description string
Ignore bool `yaml:"ignore"`
Expand Down Expand Up @@ -213,7 +214,7 @@ func (p *Parser) parseMetricsView(ctx context.Context, node *Node) error {
}

names := make(map[string]bool)
columns := make(map[string]bool)

for i, dim := range tmp.Dimensions {
if dim == nil || dim.Ignore {
continue
Expand All @@ -233,17 +234,15 @@ 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)
}
names[lower] = true

lower = strings.ToLower(dim.Column)
if ok := columns[lower]; ok {
return fmt.Errorf("found duplicate dimension column name %q", dim.Column)
}
columns[lower] = true
begelundmuller marked this conversation as resolved.
Show resolved Hide resolved
}

measureCount := 0
Expand All @@ -265,10 +264,6 @@ func (p *Parser) parseMetricsView(ctx context.Context, node *Node) error {
}
names[lower] = true

if ok := columns[lower]; ok {
return fmt.Errorf("measure name %q coincides with a dimension column name", measure.Name)
}

if measure.FormatPreset != "" && measure.FormatD3 != "" {
return fmt.Errorf(`cannot set both "format_preset" and "format_d3" for a measure`)
}
Expand Down Expand Up @@ -424,6 +419,7 @@ func (p *Parser) parseMetricsView(ctx context.Context, node *Node) error {
spec.Dimensions = append(spec.Dimensions, &runtimev1.MetricsViewSpec_DimensionV2{
Name: dim.Name,
Column: dim.Column,
Expression: dim.Expression,
Label: dim.Label,
Description: dim.Description,
Unnest: dim.Unnest,
Expand Down
12 changes: 8 additions & 4 deletions runtime/compilers/rillv1/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ materialize: true
model: m2
dimensions:
- name: a
column: a
measures:
- name: b
expression: count(*)
Expand Down Expand Up @@ -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(*)"},
Expand Down Expand Up @@ -841,6 +842,7 @@ dashboards:
table: t1
dimensions:
- name: a
column: a
measures:
- name: b
expression: count(*)
Expand All @@ -850,6 +852,7 @@ measures:
table: t2
dimensions:
- name: a
column: a
measures:
- name: b
expression: count(*)
Expand All @@ -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(*)"},
Expand All @@ -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(*)"},
Expand Down Expand Up @@ -971,6 +974,7 @@ func TestMetricsViewAvoidSelfCyclicRef(t *testing.T) {
model: d1
dimensions:
- name: a
column: a
measures:
- name: b
expression: count(*)
Expand All @@ -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(*)"},
Expand Down
9 changes: 2 additions & 7 deletions runtime/queries/column_timeseries.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,8 @@ func (q *ColumnTimeseries) Resolve(ctx context.Context, rt *runtime.Runtime, ins
}

return olap.WithConnection(ctx, priority, false, false, func(ctx context.Context, ensuredCtx context.Context, _ *sql.Conn) error {
filter, args, err := buildFilterClauseForMetricsViewFilter(q.MetricsView, q.MetricsViewFilter, olap.Dialect(), q.MetricsViewPolicy)
if err != nil {
return err
}
if filter != "" {
filter = "WHERE 1=1 " + filter
}
filter := ""
var args []any

measures := normaliseMeasures(q.Measures, q.Pixels != 0)
dateTruncSpecifier := convertToDateTruncSpecifier(timeRange.Interval)
Expand Down
Loading
Loading