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

AdityaHegde · 2023-12-14T12:59:24Z

Add expression to dashboard yaml.
Update all queries and test for dimension expression.
Support unnest with dimension expressions.
Test on driud.
Introduce proper validation.

runtime/compilers/rillv1/parse_metrics_view.go

begelundmuller · 2023-12-21T07:09:19Z

runtime/queries/metricsview.go

@@ -297,7 +297,7 @@ func columnIdentifierExpression(mv *runtimev1.MetricsViewSpec, aliases []*runtim
 	// check if identifier is a dimension
 	for _, dim := range mv.Dimensions {
 		if dim.Name == name {
-			return safeName(metricsViewDimensionColumn(dim)), true
+			return metricsViewDimensionExpression(dim), true


This will get templated into the where clause:

Might it be problematic to put a free form expression in here? I think at the least it will need to be wrapped in parentheses

Would it be better to template in just the dimension name (which the expression is aliased to in the SELECT clause)? But not sure if that works on Druid

The problem with this is for queries where the dimension is not selected we would have issues with the column not being there. The other option is to only add it if is in the select caluse. That made these builder functions too complex.

As for adding parentheses, it gets added when the identifier is actually used.

I guess we are missing a validation to make sure there isnt sql injection happening.

The case where the dim is not selected makes sense. Agree on not making it conditional on the select clause, seems to complicated.

About the parentheses, imagine if the expression is col1 = 10 and the filter is dim1 = true, then you're saying the parenthesis will be added such that the where clause becomes (col1 = 10) = true (correct) and not col1 = 10 = true (which would error)?

How could SQL injection happen? For the where filters, I thought we were doing lookups to resolve identifiers and using ? and args to pass values?

Yes all binary expressions have it. like and in do not seem to have it. Lemme add that.

It could happen in the dimension expression, count(*)";drop table blah; -- or something like that.

runtime/queries/metricsview.go

runtime/reconcilers/metrics_view.go

begelundmuller · 2023-12-21T10:32:55Z

runtime/compilers/rillv1/parse_metrics_view.go

 		Column      string
+		Expression  string


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)

begelundmuller · 2023-12-21T10:38:14Z

runtime/queries/metricsview.go

@@ -297,7 +297,7 @@ func columnIdentifierExpression(mv *runtimev1.MetricsViewSpec, aliases []*runtim
 	// check if identifier is a dimension
 	for _, dim := range mv.Dimensions {
 		if dim.Name == name {
-			return safeName(metricsViewDimensionColumn(dim)), true
+			return metricsViewDimensionExpression(dim), true


The case where the dim is not selected makes sense. Agree on not making it conditional on the select clause, seems to complicated.

About the parentheses, imagine if the expression is col1 = 10 and the filter is dim1 = true, then you're saying the parenthesis will be added such that the where clause becomes (col1 = 10) = true (correct) and not col1 = 10 = true (which would error)?

How could SQL injection happen? For the where filters, I thought we were doing lookups to resolve identifiers and using ? and args to pass values?

begelundmuller · 2023-12-21T10:39:30Z

runtime/reconcilers/metrics_view.go

+	if d.Expression == "" {
+		if d.Column == "" {
+			return nil


Shouldn't this be an error? (And ideally checked also at parser level)

True. I guess we will never hit this without the parser running and dont need backwards compatibility right?

begelundmuller · 2023-12-21T10:40:34Z

runtime/reconcilers/metrics_view.go

+	}
+
+	err := olap.Exec(ctx, &drivers.Statement{
+		Query:  fmt.Sprintf("SELECT %s FROM %s GROUP BY 1", d.Expression, safeSQLName(t.Name)),


Should we put parentheses around the expression, i.e. SELECT (%s) ...? In case the expression is a sub-query

Might this also be needed elsewhere?

Yes adding them

begelundmuller

LGTM!

AdityaHegde added 2 commits December 14, 2023 18:28

Adding support for dimension expression parsing

eb44ebb

Fix aggregation and toplist for dimension expressions

c05d208

AdityaHegde self-assigned this Dec 14, 2023

AdityaHegde changed the title ~~Adityahegde/dimension expressions~~ [Dashboard] Support arbitrary non-aggregate expressions in dimensions Dec 14, 2023

AdityaHegde added 8 commits December 14, 2023 22:00

Update toplist API

0dcce71

Merge branch 'main' into adityahegde/dimension-expressions

3ac3cc7

Integrate with new filter expression

475b04b

Add support for unnest

ca2978e

Update for druid

6bd90a6

Fix tests

8368870

Adding validation for aggreagates

9d2e65f

Add more tests

8c92875

AdityaHegde requested a review from begelundmuller December 20, 2023 12:42

AdityaHegde marked this pull request as ready for review December 20, 2023 12:42

AdityaHegde added 2 commits December 21, 2023 10:55

Fix aggreagation query

ab7ace2

Merge branch 'main' into adityahegde/dimension-expressions

d184e1f

begelundmuller requested changes Dec 21, 2023

View reviewed changes

PR comments

b2a89f5

AdityaHegde force-pushed the adityahegde/dimension-expressions branch from dea5101 to b2a89f5 Compare December 21, 2023 09:20

Fix escaping issues

20fcff9

begelundmuller requested changes Dec 21, 2023

View reviewed changes

More PR comments

82c2e40

AdityaHegde force-pushed the adityahegde/dimension-expressions branch from 2c8d821 to 82c2e40 Compare December 21, 2023 12:06

begelundmuller approved these changes Dec 22, 2023

View reviewed changes

begelundmuller merged commit 018ae8a into main Dec 22, 2023
7 checks passed

begelundmuller deleted the adityahegde/dimension-expressions branch December 22, 2023 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AdityaHegde commented Dec 14, 2023 •

edited

Loading

begelundmuller Dec 21, 2023

AdityaHegde Dec 21, 2023

begelundmuller Dec 21, 2023

AdityaHegde Dec 21, 2023

begelundmuller Dec 21, 2023

begelundmuller Dec 21, 2023

begelundmuller Dec 21, 2023

AdityaHegde Dec 21, 2023

begelundmuller Dec 21, 2023

begelundmuller Dec 21, 2023

AdityaHegde Dec 21, 2023

begelundmuller left a comment

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

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

Conversation

AdityaHegde commented Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begelundmuller left a comment

Choose a reason for hiding this comment

AdityaHegde commented Dec 14, 2023 •

edited

Loading