From 8b38b31ca835e3f165fc2a1033694532a6dbe197 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Mon, 14 Sep 2026 16:18:32 +0530 Subject: [PATCH 1/8] fix: missing unnext support for bigquery and snowflake: --- runtime/drivers/bigquery/dialect.go | 18 ++++ runtime/drivers/bigquery/olap_test.go | 110 +++++++++++++++++++++++++ runtime/drivers/snowflake/dialect.go | 23 ++++++ runtime/metricsview/ast_unnest_test.go | 87 +++++++++++++++++++ 4 files changed, 238 insertions(+) create mode 100644 runtime/metricsview/ast_unnest_test.go diff --git a/runtime/drivers/bigquery/dialect.go b/runtime/drivers/bigquery/dialect.go index 55e1f32eb252..ac4451ffc6d9 100644 --- a/runtime/drivers/bigquery/dialect.go +++ b/runtime/drivers/bigquery/dialect.go @@ -70,6 +70,24 @@ func (d *dialect) OrderByAliasExpression(name string, desc bool) string { return res } +func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) { + expr, err := d.MetricsViewDimensionExpression(dim) + if err != nil { + return "", "", fmt.Errorf("failed to get dimension expression: %w", err) + } + alias := d.EscapeAlias(dim.Name) + if !dim.Unnest { + return fmt.Sprintf(`(%s) AS %s`, expr, alias), "", nil + } + unnestColName := d.EscapeIdentifier(drivers.TempName(fmt.Sprintf("unnested_%s_", dim.Name))) + return fmt.Sprintf(`%s AS %s`, unnestColName, alias), fmt.Sprintf(`, UNNEST(%s) AS %s`, expr, unnestColName), nil +} + +// LateralUnnest returns a comma join with UNNEST. BigQuery exposes each array element directly under the alias, so there is no tuple to index into. +func (d *dialect) LateralUnnest(expr, _, colName string) (tbl string, tupleStyle, auto bool, err error) { + return fmt.Sprintf(`UNNEST(%s) AS %s`, expr, d.EscapeIdentifier(colName)), false, false, nil +} + func (d *dialect) JoinOnExpression(lhs, rhs string) string { // BigQuery requires plain equality for FULL joins return fmt.Sprintf("coalesce(CAST(%s AS STRING), '__rill_sentinel__') = coalesce(CAST(%s AS STRING), '__rill_sentinel__')", lhs, rhs) diff --git a/runtime/drivers/bigquery/olap_test.go b/runtime/drivers/bigquery/olap_test.go index a003847d1c21..a5864fbfd343 100644 --- a/runtime/drivers/bigquery/olap_test.go +++ b/runtime/drivers/bigquery/olap_test.go @@ -2,12 +2,14 @@ package bigquery_test import ( "context" + "fmt" "testing" "time" "github.com/google/uuid" runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" + "github.com/rilldata/rill/runtime/metricsview" "github.com/rilldata/rill/runtime/pkg/activity" "github.com/rilldata/rill/runtime/storage" "github.com/rilldata/rill/runtime/testruntime" @@ -121,6 +123,114 @@ func TestOLAP(t *testing.T) { } } +func TestUnnestDimension(t *testing.T) { + testmode.Expensive(t) + _, olap := acquireTestBigQuery(t) + + mv := &runtimev1.MetricsViewSpec{ + Database: "rilldata", + DatabaseSchema: "integration_test", + Table: "all_datatypes", + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "array_string_col", Column: "array_string_col", Unnest: true}, + {Name: "int_col", Column: "int_col"}, + }, + Measures: []*runtimev1.MetricsViewSpec_Measure{ + {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, + }, + } + + // Same query shape as the executor's dimension validation. + dialect := olap.Dialect() + escapeTable := dialect.EscapeTable(mv.Database, mv.DatabaseSchema, mv.Table) + sel, unnestClause, err := dialect.DimensionSelect(escapeTable, mv.Dimensions[0]) + require.NoError(t, err) + err = olap.Exec(t.Context(), &drivers.Statement{Query: fmt.Sprintf("SELECT %s FROM %s %s GROUP BY 1", sel, escapeTable, unnestClause), DryRun: true}) + require.NoError(t, err) + + // Control: rows whose array contains 'sample1', computed without the metrics view code paths. + control := queryRows(t, olap, "SELECT COUNT(*) AS count FROM `rilldata.integration_test.all_datatypes` WHERE 'sample1' IN UNNEST(array_string_col)", nil) + require.Len(t, control, 1) + matching := control[0]["count"].(int64) + require.Greater(t, matching, int64(0)) + + arrayEq := func(op metricsview.Operator, val any) *metricsview.Expression { + return &metricsview.Expression{Condition: &metricsview.Condition{ + Operator: op, + Expressions: []*metricsview.Expression{{Name: "array_string_col"}, {Value: val}}, + }} + } + + tests := []struct { + name string + qry *metricsview.Query + want []map[string]any + }{ + { + name: "group by unnest dimension", + qry: &metricsview.Query{ + Dimensions: []metricsview.Dimension{{Name: "array_string_col"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + }, + want: []map[string]any{{"array_string_col": "sample1", "count": matching}}, + }, + { + name: "eq filter on unnest dimension", + qry: &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "count"}}, + Where: arrayEq(metricsview.OperatorEq, "sample1"), + }, + want: []map[string]any{{"count": matching}}, + }, + { + name: "in filter on unnest dimension", + qry: &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "count"}}, + Where: arrayEq(metricsview.OperatorIn, []any{"sample1", "missing"}), + }, + want: []map[string]any{{"count": matching}}, + }, + { + name: "eq filter on unnest dimension with no match", + qry: &metricsview.Query{ + Measures: []metricsview.Measure{{Name: "count"}}, + Where: arrayEq(metricsview.OperatorEq, "missing"), + }, + want: []map[string]any{{"count": int64(0)}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.qry.MetricsView = "all_datatypes" + ast, err := metricsview.NewAST(mv, allowAllSecurity{}, tt.qry, dialect) + require.NoError(t, err) + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Equal(t, tt.want, queryRows(t, olap, sql, args)) + }) + } +} + +func queryRows(t *testing.T, olap drivers.OLAPStore, query string, args []any) []map[string]any { + rows, err := olap.Query(t.Context(), &drivers.Statement{Query: query, Args: args}) + require.NoError(t, err) + defer rows.Close() + var res []map[string]any + for rows.Next() { + row := make(map[string]any) + require.NoError(t, rows.MapScan(row)) + res = append(res, row) + } + require.NoError(t, rows.Err()) + return res +} + +type allowAllSecurity struct{} + +func (allowAllSecurity) CanAccessField(string) bool { return true } +func (allowAllSecurity) RowFilter() string { return "" } +func (allowAllSecurity) QueryFilter() *runtimev1.Expression { return nil } + func TestEmptyRows(t *testing.T) { testmode.Expensive(t) _, olap := acquireTestBigQuery(t) diff --git a/runtime/drivers/snowflake/dialect.go b/runtime/drivers/snowflake/dialect.go index a4174c45e59b..a328b1f5caf5 100644 --- a/runtime/drivers/snowflake/dialect.go +++ b/runtime/drivers/snowflake/dialect.go @@ -43,6 +43,29 @@ func (d *dialect) OrderByAliasExpression(name string, desc bool) string { return res } +func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) { + expr, err := d.MetricsViewDimensionExpression(dim) + if err != nil { + return "", "", fmt.Errorf("failed to get dimension expression: %w", err) + } + alias := d.EscapeAlias(dim.Name) + if !dim.Unnest { + return fmt.Sprintf(`(%s) AS %s`, expr, alias), "", nil + } + unnestCol := drivers.TempName(fmt.Sprintf("unnested_%s_", dim.Name)) + tbl, _, _, err := d.LateralUnnest(expr, drivers.TempName("tbl"), unnestCol) + if err != nil { + return "", "", err + } + return fmt.Sprintf(`%s AS %s`, d.EscapeIdentifier(unnestCol), alias), ", " + tbl, nil +} + +// LateralUnnest wraps FLATTEN in an inline view so the element is exposed under colName instead of FLATTEN's fixed VALUE column. +// VALUE is a VARIANT for semi-structured arrays and would otherwise surface as JSON-encoded text, so it is cast to VARCHAR. +func (d *dialect) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) { + return fmt.Sprintf(`LATERAL (SELECT VALUE::VARCHAR AS %s FROM TABLE(FLATTEN(INPUT => %s))) %s`, d.EscapeIdentifier(colName), expr, tableAlias), true, false, nil +} + func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { if tz == "UTC" || tz == "Etc/UTC" { tz = "" diff --git a/runtime/metricsview/ast_unnest_test.go b/runtime/metricsview/ast_unnest_test.go new file mode 100644 index 000000000000..02e22514db10 --- /dev/null +++ b/runtime/metricsview/ast_unnest_test.go @@ -0,0 +1,87 @@ +package metricsview + +import ( + "testing" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/rilldata/rill/runtime/drivers" + "github.com/rilldata/rill/runtime/drivers/bigquery" + "github.com/rilldata/rill/runtime/drivers/snowflake" + "github.com/stretchr/testify/require" +) + +func TestUnnestSQL(t *testing.T) { + mv := &runtimev1.MetricsViewSpec{ + Table: "test_table", + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "tags", Column: "tags", Unnest: true}, + {Name: "city", Column: "city"}, + }, + Measures: []*runtimev1.MetricsViewSpec_Measure{ + {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, + }, + } + sec := skipMetricsViewSecurity{} + + tagsEqA := &Expression{Condition: &Condition{ + Operator: OperatorEq, + Expressions: []*Expression{{Name: "tags"}, {Value: "a"}}, + }} + + tests := []struct { + name string + dialect drivers.Dialect + dims []Dimension + where *Expression + wantSQL string + wantArgs []any + }{ + { + name: "bigquery: group by unnest dim", + dialect: bigquery.DialectBigQuery, + dims: []Dimension{{Name: "tags"}}, + wantSQL: "SELECT (`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` GROUP BY 1", + }, + { + name: "bigquery: filter on unnest dim not in select", + dialect: bigquery.DialectBigQuery, + dims: []Dimension{{Name: "city"}}, + where: tagsEqA, + wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` WHERE ((`tags`) = ?) GROUP BY 1", + wantArgs: []any{"a"}, + }, + { + name: "snowflake: group by unnest dim", + dialect: snowflake.DialectSnowflake, + dims: []Dimension{{Name: "tags"}}, + wantSQL: `SELECT (t0.tags) AS "tags", (count(*)) AS "count" FROM test_table, LATERAL (SELECT VALUE::VARCHAR AS tags FROM TABLE(FLATTEN(INPUT => tags))) t0 GROUP BY 1`, + }, + { + name: "snowflake: filter on unnest dim not in select", + dialect: snowflake.DialectSnowflake, + dims: []Dimension{{Name: "city"}}, + where: tagsEqA, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE EXISTS (SELECT 1 FROM LATERAL (SELECT VALUE::VARCHAR AS tags FROM TABLE(FLATTEN(INPUT => tags))) t0 WHERE ((t0.tags) = ?)) GROUP BY 1`, + wantArgs: []any{"a"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + qry := &Query{ + MetricsView: "test", + Dimensions: tt.dims, + Measures: []Measure{{Name: "count"}}, + Where: tt.where, + } + + ast, err := NewAST(mv, sec, qry, tt.dialect) + require.NoError(t, err) + + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Equal(t, tt.wantSQL, sql) + require.Equal(t, tt.wantArgs, args) + }) + } +} From 86a2d02c21250015d04298adcbdb362572380957 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:46:35 +0530 Subject: [PATCH 2/8] snowflake fixes --- runtime/drivers/bigquery/dialect.go | 9 +- runtime/drivers/bigquery/olap_test.go | 88 ++++++++++----- runtime/drivers/dialect.go | 14 +++ runtime/drivers/snowflake/dialect.go | 21 +++- runtime/drivers/snowflake/olap_test.go | 145 +++++++++++++++++++++++++ runtime/metricsview/ast.go | 2 +- runtime/metricsview/ast_unnest_test.go | 19 +++- runtime/metricsview/astexpr.go | 19 ++-- 8 files changed, 267 insertions(+), 50 deletions(-) diff --git a/runtime/drivers/bigquery/dialect.go b/runtime/drivers/bigquery/dialect.go index ac4451ffc6d9..056ae34aec50 100644 --- a/runtime/drivers/bigquery/dialect.go +++ b/runtime/drivers/bigquery/dialect.go @@ -83,9 +83,12 @@ func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimen return fmt.Sprintf(`%s AS %s`, unnestColName, alias), fmt.Sprintf(`, UNNEST(%s) AS %s`, expr, unnestColName), nil } -// LateralUnnest returns a comma join with UNNEST. BigQuery exposes each array element directly under the alias, so there is no tuple to index into. -func (d *dialect) LateralUnnest(expr, _, colName string) (tbl string, tupleStyle, auto bool, err error) { - return fmt.Sprintf(`UNNEST(%s) AS %s`, expr, d.EscapeIdentifier(colName)), false, false, nil +// LateralUnnest wraps each array element in a STRUCT so it can be addressed as tableAlias.colName. +// BigQuery's UNNEST exposes scalar elements directly under the alias, which the AST cannot address in tuple style. +// Tuple style is required so that filters on unselected unnest dimensions become EXISTS subqueries instead of joins that duplicate rows. +func (d *dialect) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) { + elem := d.EscapeIdentifier(tableAlias + "_elem") + return fmt.Sprintf(`UNNEST(ARRAY(SELECT AS STRUCT %s AS %s FROM UNNEST(%s) AS %s)) AS %s`, elem, d.EscapeIdentifier(colName), expr, elem, d.EscapeIdentifier(tableAlias)), true, false, nil } func (d *dialect) JoinOnExpression(lhs, rhs string) string { diff --git a/runtime/drivers/bigquery/olap_test.go b/runtime/drivers/bigquery/olap_test.go index a5864fbfd343..8d1d1f0c98f4 100644 --- a/runtime/drivers/bigquery/olap_test.go +++ b/runtime/drivers/bigquery/olap_test.go @@ -127,13 +127,23 @@ func TestUnnestDimension(t *testing.T) { testmode.Expensive(t) _, olap := acquireTestBigQuery(t) + // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + name := "test_unnest_" + uuid.New().String()[:8] + table := "`rilldata.integration_test." + name + "`" + t.Cleanup(func() { + err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + table}) + require.NoError(t, err) + }) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + table + " AS SELECT id, tags FROM UNNEST(ARRAY>>[(1, ['a', 'b']), (2, ['b']), (3, ['c']), (4, ARRAY[])])"}) + require.NoError(t, err) + mv := &runtimev1.MetricsViewSpec{ Database: "rilldata", DatabaseSchema: "integration_test", - Table: "all_datatypes", + Table: name, Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ - {Name: "array_string_col", Column: "array_string_col", Unnest: true}, - {Name: "int_col", Column: "int_col"}, + {Name: "tags", Column: "tags", Unnest: true}, + {Name: "id", Column: "id"}, }, Measures: []*runtimev1.MetricsViewSpec_Measure{ {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, @@ -148,18 +158,15 @@ func TestUnnestDimension(t *testing.T) { err = olap.Exec(t.Context(), &drivers.Statement{Query: fmt.Sprintf("SELECT %s FROM %s %s GROUP BY 1", sel, escapeTable, unnestClause), DryRun: true}) require.NoError(t, err) - // Control: rows whose array contains 'sample1', computed without the metrics view code paths. - control := queryRows(t, olap, "SELECT COUNT(*) AS count FROM `rilldata.integration_test.all_datatypes` WHERE 'sample1' IN UNNEST(array_string_col)", nil) - require.Len(t, control, 1) - matching := control[0]["count"].(int64) - require.Greater(t, matching, int64(0)) - - arrayEq := func(op metricsview.Operator, val any) *metricsview.Expression { + tagsFilter := func(op metricsview.Operator, val any) *metricsview.Expression { return &metricsview.Expression{Condition: &metricsview.Condition{ Operator: op, - Expressions: []*metricsview.Expression{{Name: "array_string_col"}, {Value: val}}, + Expressions: []*metricsview.Expression{{Name: "tags"}, {Value: val}}, }} } + count := func(where *metricsview.Expression) *metricsview.Query { + return &metricsview.Query{Measures: []metricsview.Measure{{Name: "count"}}, Where: where} + } tests := []struct { name string @@ -169,39 +176,60 @@ func TestUnnestDimension(t *testing.T) { { name: "group by unnest dimension", qry: &metricsview.Query{ - Dimensions: []metricsview.Dimension{{Name: "array_string_col"}}, + Dimensions: []metricsview.Dimension{{Name: "tags"}}, Measures: []metricsview.Measure{{Name: "count"}}, + Sort: []metricsview.Sort{{Name: "tags"}}, + }, + want: []map[string]any{ + {"tags": "a", "count": int64(1)}, + {"tags": "b", "count": int64(2)}, + {"tags": "c", "count": int64(1)}, }, - want: []map[string]any{{"array_string_col": "sample1", "count": matching}}, }, { - name: "eq filter on unnest dimension", - qry: &metricsview.Query{ - Measures: []metricsview.Measure{{Name: "count"}}, - Where: arrayEq(metricsview.OperatorEq, "sample1"), - }, - want: []map[string]any{{"count": matching}}, + // Row 1 matches both values but must be counted once. + name: "in filter counts each source row once", + qry: count(tagsFilter(metricsview.OperatorIn, []any{"a", "b"})), + want: []map[string]any{{"count": int64(2)}}, }, { - name: "in filter on unnest dimension", - qry: &metricsview.Query{ - Measures: []metricsview.Measure{{Name: "count"}}, - Where: arrayEq(metricsview.OperatorIn, []any{"sample1", "missing"}), - }, - want: []map[string]any{{"count": matching}}, + // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + name: "nin filter excludes rows containing any listed value", + qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), + want: []map[string]any{{"count": int64(3)}}, + }, + { + name: "eq filter", + qry: count(tagsFilter(metricsview.OperatorEq, "b")), + want: []map[string]any{{"count": int64(2)}}, }, { - name: "eq filter on unnest dimension with no match", + name: "neq filter excludes rows containing the value", + qry: count(tagsFilter(metricsview.OperatorNeq, "b")), + want: []map[string]any{{"count": int64(2)}}, + }, + { + name: "eq filter with no match", + qry: count(tagsFilter(metricsview.OperatorEq, "missing")), + want: []map[string]any{{"count": int64(0)}}, + }, + { + name: "filter combined with group by on another dimension", qry: &metricsview.Query{ - Measures: []metricsview.Measure{{Name: "count"}}, - Where: arrayEq(metricsview.OperatorEq, "missing"), + Dimensions: []metricsview.Dimension{{Name: "id"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + Where: tagsFilter(metricsview.OperatorIn, []any{"a", "b"}), + Sort: []metricsview.Sort{{Name: "id"}}, + }, + want: []map[string]any{ + {"id": int64(1), "count": int64(1)}, + {"id": int64(2), "count": int64(1)}, }, - want: []map[string]any{{"count": int64(0)}}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.qry.MetricsView = "all_datatypes" + tt.qry.MetricsView = name ast, err := metricsview.NewAST(mv, allowAllSecurity{}, tt.qry, dialect) require.NoError(t, err) sql, args, err := ast.SQL() diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index 445f2ee5323a..80acd285b969 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -51,6 +51,12 @@ type Dialect interface { GetArrayContainsFunction() (string, error) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) + // UnnestedColumn returns the expression for the array element exposed by LateralUnnest in tuple style. + UnnestedColumn(tableAlias, colName string) string + // ArrayAnyExpression returns fragments for a condition that is true if any element of arrExpr satisfies it. + // The condition on a single element is written between open and close and references the element as elem. + // ok is false if the dialect has no such expression, in which case a correlated EXISTS subquery over LateralUnnest is used. + ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) UnnestSQLSuffix(tbl string) string // AutoUnnest wraps an expression so the dialect unnests it automatically (used when LateralUnnest reports auto == true). AutoUnnest(expr string) string @@ -218,6 +224,14 @@ func (b *BaseDialect) LateralUnnest(expr, tableAlias, colName string) (tbl strin return fmt.Sprintf(`LATERAL UNNEST(%s) %s(%s)`, expr, tableAlias, b.escapeIdentifier(colName)), true, false, nil } +func (b *BaseDialect) UnnestedColumn(tableAlias, colName string) string { + return b.EscapeMember(tableAlias, colName) +} + +func (b *BaseDialect) ArrayAnyExpression(_, _ string) (open, elem, closing string, ok bool) { + return "", "", "", false +} + func (b *BaseDialect) UnnestSQLSuffix(tbl string) string { return fmt.Sprintf(", %s", tbl) } diff --git a/runtime/drivers/snowflake/dialect.go b/runtime/drivers/snowflake/dialect.go index a328b1f5caf5..ba2aec3d39e8 100644 --- a/runtime/drivers/snowflake/dialect.go +++ b/runtime/drivers/snowflake/dialect.go @@ -53,17 +53,28 @@ func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimen return fmt.Sprintf(`(%s) AS %s`, expr, alias), "", nil } unnestCol := drivers.TempName(fmt.Sprintf("unnested_%s_", dim.Name)) - tbl, _, _, err := d.LateralUnnest(expr, drivers.TempName("tbl"), unnestCol) + tableAlias := drivers.TempName("tbl") + tbl, _, _, err := d.LateralUnnest(expr, tableAlias, unnestCol) if err != nil { return "", "", err } - return fmt.Sprintf(`%s AS %s`, d.EscapeIdentifier(unnestCol), alias), ", " + tbl, nil + return fmt.Sprintf(`%s AS %s`, d.UnnestedColumn(tableAlias, unnestCol), alias), ", " + tbl, nil } -// LateralUnnest wraps FLATTEN in an inline view so the element is exposed under colName instead of FLATTEN's fixed VALUE column. -// VALUE is a VARIANT for semi-structured arrays and would otherwise surface as JSON-encoded text, so it is cast to VARCHAR. +// LateralUnnest aliases every FLATTEN output column so the element is addressable as tableAlias.colName. +// FLATTEN cannot be wrapped in an inline view because Snowflake does not resolve the outer array column inside it. func (d *dialect) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) { - return fmt.Sprintf(`LATERAL (SELECT VALUE::VARCHAR AS %s FROM TABLE(FLATTEN(INPUT => %s))) %s`, d.EscapeIdentifier(colName), expr, tableAlias), true, false, nil + return fmt.Sprintf(`LATERAL FLATTEN(INPUT => %s) %s (seq, key, path, index, %s, this)`, expr, tableAlias, d.EscapeIdentifier(colName)), true, false, nil +} + +// UnnestedColumn casts the element to VARCHAR. FLATTEN yields VARIANT elements for semi-structured arrays, which the driver returns as JSON-encoded text. +func (d *dialect) UnnestedColumn(tableAlias, colName string) string { + return d.EscapeMember(tableAlias, colName) + "::VARCHAR" +} + +// ArrayAnyExpression uses FILTER because Snowflake rejects correlated FLATTEN inside EXISTS subqueries. +func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { + return fmt.Sprintf("(ARRAY_SIZE(FILTER(%s, %s -> ", arrExpr, elemAlias), elemAlias + "::VARCHAR", ")) > 0)", true } func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { diff --git a/runtime/drivers/snowflake/olap_test.go b/runtime/drivers/snowflake/olap_test.go index 0ab31bca2b0f..0fdb96b81258 100644 --- a/runtime/drivers/snowflake/olap_test.go +++ b/runtime/drivers/snowflake/olap_test.go @@ -1,12 +1,17 @@ package snowflake_test import ( + "context" "encoding/json" + "fmt" "strings" "testing" "time" + "github.com/google/uuid" + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" + "github.com/rilldata/rill/runtime/metricsview" "github.com/rilldata/rill/runtime/pkg/activity" "github.com/rilldata/rill/runtime/storage" "github.com/rilldata/rill/runtime/testruntime" @@ -184,6 +189,146 @@ func TestDryRun(t *testing.T) { require.NoError(t, err) } +// TestUnnestDimension creates a table in the DSN's current database and schema, so the DSN must point at a writable schema. +func TestUnnestDimension(t *testing.T) { + testmode.Expensive(t) + _, olap := acquireTestSnowflake(t) + + // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + // The driver returns NUMBER columns as strings. + name := "test_unnest_" + uuid.New().String()[:8] + t.Cleanup(func() { + err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + name}) + require.NoError(t, err) + }) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT 1 AS id, ARRAY_CONSTRUCT('a', 'b') AS tags UNION ALL SELECT 2, ARRAY_CONSTRUCT('b') UNION ALL SELECT 3, ARRAY_CONSTRUCT('c') UNION ALL SELECT 4, ARRAY_CONSTRUCT()"}) + require.NoError(t, err) + + mv := &runtimev1.MetricsViewSpec{ + Table: name, + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "tags", Column: "tags", Unnest: true}, + {Name: "id", Column: "id"}, + }, + Measures: []*runtimev1.MetricsViewSpec_Measure{ + {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, + }, + } + + // Same query shape as the executor's dimension validation. + dialect := olap.Dialect() + escapeTable := dialect.EscapeTable(mv.Database, mv.DatabaseSchema, mv.Table) + sel, unnestClause, err := dialect.DimensionSelect(escapeTable, mv.Dimensions[0]) + require.NoError(t, err) + err = olap.Exec(t.Context(), &drivers.Statement{Query: fmt.Sprintf("SELECT %s FROM %s %s GROUP BY 1", sel, escapeTable, unnestClause), DryRun: true}) + require.NoError(t, err) + + tagsFilter := func(op metricsview.Operator, val any) *metricsview.Expression { + return &metricsview.Expression{Condition: &metricsview.Condition{ + Operator: op, + Expressions: []*metricsview.Expression{{Name: "tags"}, {Value: val}}, + }} + } + count := func(where *metricsview.Expression) *metricsview.Query { + return &metricsview.Query{Measures: []metricsview.Measure{{Name: "count"}}, Where: where} + } + + tests := []struct { + name string + qry *metricsview.Query + want []map[string]any + }{ + { + name: "group by unnest dimension", + qry: &metricsview.Query{ + Dimensions: []metricsview.Dimension{{Name: "tags"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + Sort: []metricsview.Sort{{Name: "tags"}}, + }, + want: []map[string]any{ + {"tags": "a", "count": "1"}, + {"tags": "b", "count": "2"}, + {"tags": "c", "count": "1"}, + }, + }, + { + // Row 1 matches both values but must be counted once. + name: "in filter counts each source row once", + qry: count(tagsFilter(metricsview.OperatorIn, []any{"a", "b"})), + want: []map[string]any{{"count": "2"}}, + }, + { + // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + name: "nin filter excludes rows containing any listed value", + qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), + want: []map[string]any{{"count": "3"}}, + }, + { + name: "eq filter", + qry: count(tagsFilter(metricsview.OperatorEq, "b")), + want: []map[string]any{{"count": "2"}}, + }, + { + name: "neq filter excludes rows containing the value", + qry: count(tagsFilter(metricsview.OperatorNeq, "b")), + want: []map[string]any{{"count": "2"}}, + }, + { + name: "ilike filter", + qry: count(tagsFilter(metricsview.OperatorIlike, "%B%")), + want: []map[string]any{{"count": "2"}}, + }, + { + name: "eq filter with no match", + qry: count(tagsFilter(metricsview.OperatorEq, "missing")), + want: []map[string]any{{"count": "0"}}, + }, + { + name: "filter combined with group by on another dimension", + qry: &metricsview.Query{ + Dimensions: []metricsview.Dimension{{Name: "id"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + Where: tagsFilter(metricsview.OperatorIn, []any{"a", "b"}), + Sort: []metricsview.Sort{{Name: "id"}}, + }, + want: []map[string]any{ + {"id": "1", "count": "1"}, + {"id": "2", "count": "1"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.qry.MetricsView = name + ast, err := metricsview.NewAST(mv, allowAllSecurity{}, tt.qry, dialect) + require.NoError(t, err) + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Equal(t, tt.want, queryRows(t, olap, sql, args)) + }) + } +} + +func queryRows(t *testing.T, olap drivers.OLAPStore, query string, args []any) []map[string]any { + rows, err := olap.Query(t.Context(), &drivers.Statement{Query: query, Args: args}) + require.NoError(t, err) + defer rows.Close() + var res []map[string]any + for rows.Next() { + row := make(map[string]any) + require.NoError(t, rows.MapScan(row)) + res = append(res, row) + } + require.NoError(t, rows.Err()) + return res +} + +type allowAllSecurity struct{} + +func (allowAllSecurity) CanAccessField(string) bool { return true } +func (allowAllSecurity) RowFilter() string { return "" } +func (allowAllSecurity) QueryFilter() *runtimev1.Expression { return nil } + func acquireTestSnowflake(t *testing.T) (drivers.Handle, drivers.OLAPStore) { cfg := testruntime.AcquireConnector(t, "snowflake") conn, err := drivers.Open("snowflake", "", "default", cfg, storage.MustNew(t.TempDir(), nil), activity.NewNoopClient(), zap.NewNop()) diff --git a/runtime/metricsview/ast.go b/runtime/metricsview/ast.go index 49b823dde57c..782a887c8dd6 100644 --- a/runtime/metricsview/ast.go +++ b/runtime/metricsview/ast.go @@ -266,7 +266,7 @@ func NewAST(mv *runtimev1.MetricsViewSpec, sec MetricsViewSecurity, qry *Query, } else { ast.unnests = append(ast.unnests, tblWithAlias) if tupleStyle { - f.Expr = ast.Dialect.EscapeMember(unnestAlias, f.Name) + f.Expr = ast.Dialect.UnnestedColumn(unnestAlias, f.Name) } else { f.Expr = ast.Dialect.EscapeMember("", f.Name) } diff --git a/runtime/metricsview/ast_unnest_test.go b/runtime/metricsview/ast_unnest_test.go index 02e22514db10..88972c894136 100644 --- a/runtime/metricsview/ast_unnest_test.go +++ b/runtime/metricsview/ast_unnest_test.go @@ -40,30 +40,41 @@ func TestUnnestSQL(t *testing.T) { name: "bigquery: group by unnest dim", dialect: bigquery.DialectBigQuery, dims: []Dimension{{Name: "tags"}}, - wantSQL: "SELECT (`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` GROUP BY 1", + wantSQL: "SELECT (`t0`.`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(ARRAY(SELECT AS STRUCT `t0_elem` AS `tags` FROM UNNEST(`tags`) AS `t0_elem`)) AS `t0` GROUP BY 1", }, { name: "bigquery: filter on unnest dim not in select", dialect: bigquery.DialectBigQuery, dims: []Dimension{{Name: "city"}}, where: tagsEqA, - wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` WHERE ((`tags`) = ?) GROUP BY 1", + wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE EXISTS (SELECT 1 FROM UNNEST(ARRAY(SELECT AS STRUCT `t0_elem` AS `tags` FROM UNNEST(`tags`) AS `t0_elem`)) AS `t0` WHERE ((`t0`.`tags`) = ?)) GROUP BY 1", wantArgs: []any{"a"}, }, { name: "snowflake: group by unnest dim", dialect: snowflake.DialectSnowflake, dims: []Dimension{{Name: "tags"}}, - wantSQL: `SELECT (t0.tags) AS "tags", (count(*)) AS "count" FROM test_table, LATERAL (SELECT VALUE::VARCHAR AS tags FROM TABLE(FLATTEN(INPUT => tags))) t0 GROUP BY 1`, + wantSQL: `SELECT (t0.tags::VARCHAR) AS "tags", (count(*)) AS "count" FROM test_table, LATERAL FLATTEN(INPUT => tags) t0 (seq, key, path, index, tags, this) GROUP BY 1`, }, { name: "snowflake: filter on unnest dim not in select", dialect: snowflake.DialectSnowflake, dims: []Dimension{{Name: "city"}}, where: tagsEqA, - wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE EXISTS (SELECT 1 FROM LATERAL (SELECT VALUE::VARCHAR AS tags FROM TABLE(FLATTEN(INPUT => tags))) t0 WHERE ((t0.tags) = ?)) GROUP BY 1`, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE (ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) = ?))) > 0) GROUP BY 1`, wantArgs: []any{"a"}, }, + { + name: "snowflake: nin filter on unnest dim not in select", + dialect: snowflake.DialectSnowflake, + dims: []Dimension{{Name: "city"}}, + where: &Expression{Condition: &Condition{ + Operator: OperatorNin, + Expressions: []*Expression{{Name: "tags"}, {Value: []any{"a", "b"}}}, + }}, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE NOT (ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) IN (?,?)))) > 0) GROUP BY 1`, + wantArgs: []any{"a", "b"}, + }, } for _, tt := range tests { diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index a3eb41e39dd9..118181f901c2 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -296,7 +296,7 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) } var unnestColAlias string if tupleStyle { - unnestColAlias = b.ast.Dialect.EscapeMember(unnestTableAlias, left.Name) + unnestColAlias = b.ast.Dialect.UnnestedColumn(unnestTableAlias, left.Name) } else { unnestColAlias = b.ast.Dialect.EscapeAlias(left.Name) } @@ -320,18 +320,23 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) not = true } - // Output: [NOT] EXISTS (SELECT 1 FROM WHERE ) + // Evaluate the condition per source row so a row matches once even if several of its elements match. + // Output: [NOT] EXISTS (SELECT 1 FROM WHERE ), unless the dialect has a native array expression. if not { b.writeString("NOT ") } - b.writeString("EXISTS (SELECT 1 FROM ") - b.writeString(unnestFrom) - b.writeString(" WHERE ") - err = b.writeBinaryConditionInner(nil, right, unnestColAlias, op) + open, elem, closing, ok := b.ast.Dialect.ArrayAnyExpression(leftExpr, unnestTableAlias) + if !ok { + open = "EXISTS (SELECT 1 FROM " + unnestFrom + " WHERE " + elem = unnestColAlias + closing = ")" + } + b.writeString(open) + err = b.writeBinaryConditionInner(nil, right, elem, op) if err != nil { return err } - b.writeByte(')') + b.writeString(closing) return nil } From 734f7f5c7162283a850849970402acde7cd975fb Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:11:20 +0530 Subject: [PATCH 3/8] more snowflake and databricks fixes --- runtime/drivers/bigquery/dialect.go | 14 ++- runtime/drivers/clickhouse/dialect.go | 4 +- runtime/drivers/databricks/dialect.go | 13 ++- runtime/drivers/databricks/olap_test.go | 144 ++++++++++++++++++++++++ runtime/drivers/dialect.go | 7 +- runtime/drivers/duckdb/dialect.go | 4 +- runtime/drivers/snowflake/dialect.go | 6 + runtime/metricsview/ast_unnest_test.go | 21 +++- runtime/metricsview/astexpr.go | 52 +++------ runtime/metricsview/astexpr_test.go | 55 +++++++++ 10 files changed, 271 insertions(+), 49 deletions(-) diff --git a/runtime/drivers/bigquery/dialect.go b/runtime/drivers/bigquery/dialect.go index 056ae34aec50..18171fde60fc 100644 --- a/runtime/drivers/bigquery/dialect.go +++ b/runtime/drivers/bigquery/dialect.go @@ -83,12 +83,14 @@ func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimen return fmt.Sprintf(`%s AS %s`, unnestColName, alias), fmt.Sprintf(`, UNNEST(%s) AS %s`, expr, unnestColName), nil } -// LateralUnnest wraps each array element in a STRUCT so it can be addressed as tableAlias.colName. -// BigQuery's UNNEST exposes scalar elements directly under the alias, which the AST cannot address in tuple style. -// Tuple style is required so that filters on unselected unnest dimensions become EXISTS subqueries instead of joins that duplicate rows. -func (d *dialect) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) { - elem := d.EscapeIdentifier(tableAlias + "_elem") - return fmt.Sprintf(`UNNEST(ARRAY(SELECT AS STRUCT %s AS %s FROM UNNEST(%s) AS %s)) AS %s`, elem, d.EscapeIdentifier(colName), expr, elem, d.EscapeIdentifier(tableAlias)), true, false, nil +// LateralUnnest returns a comma join with UNNEST. BigQuery exposes each element directly under the alias, so there is no tuple to index into. +func (d *dialect) LateralUnnest(expr, _, colName string) (tbl string, tupleStyle, auto bool, err error) { + return fmt.Sprintf(`UNNEST(%s) AS %s`, expr, d.EscapeIdentifier(colName)), false, false, nil +} + +func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { + elem = d.EscapeIdentifier(elemAlias) + return fmt.Sprintf("EXISTS (SELECT 1 FROM UNNEST(%s) AS %s WHERE ", arrExpr, elem), elem, ")", true } func (d *dialect) JoinOnExpression(lhs, rhs string) string { diff --git a/runtime/drivers/clickhouse/dialect.go b/runtime/drivers/clickhouse/dialect.go index b32f9622624b..e6c3bad22653 100644 --- a/runtime/drivers/clickhouse/dialect.go +++ b/runtime/drivers/clickhouse/dialect.go @@ -82,7 +82,9 @@ func (d *dialect) AutoUnnest(expr string) string { func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } -func (d *dialect) GetArrayContainsFunction() (string, error) { return "hasAny", nil } +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { + return fmt.Sprintf("hasAny(%s, [%s])", arrExpr, valuesExpr), nil +} func (d *dialect) CastToDataType(typ runtimev1.Type_Code) (string, error) { switch typ { diff --git a/runtime/drivers/databricks/dialect.go b/runtime/drivers/databricks/dialect.go index b9b216f59f6a..5c320ecf7d91 100644 --- a/runtime/drivers/databricks/dialect.go +++ b/runtime/drivers/databricks/dialect.go @@ -71,14 +71,25 @@ func (d *dialect) DimensionSelect(escapeTable string, dim *runtimev1.MetricsView return sel, fmt.Sprintf(` LATERAL VIEW EXPLODE(%s) %s AS %s`, dim.Expression, unnestTableName, unnestColName), nil } +// LateralUnnest uses tuple style so the element is referenced as tableAlias.colName; an unqualified colName is ambiguous when it matches the source column. func (d *dialect) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) { - return fmt.Sprintf(`LATERAL VIEW EXPLODE(%s) %s AS %s`, expr, tableAlias, d.EscapeIdentifier(colName)), false, false, nil + return fmt.Sprintf(`LATERAL VIEW EXPLODE(%s) %s AS %s`, expr, tableAlias, d.EscapeIdentifier(colName)), true, false, nil } func (d *dialect) UnnestSQLSuffix(tbl string) string { return fmt.Sprintf(" %s", tbl) } +func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { + return fmt.Sprintf("EXISTS(%s, %s -> ", arrExpr, elemAlias), elemAlias, ")", true +} + +func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } + +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { + return fmt.Sprintf("arrays_overlap(%s, array(%s))", arrExpr, valuesExpr), nil +} + func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { if tz == "UTC" || tz == "Etc/UTC" { tz = "" diff --git a/runtime/drivers/databricks/olap_test.go b/runtime/drivers/databricks/olap_test.go index 11f7b7c0d5c4..840566410dfe 100644 --- a/runtime/drivers/databricks/olap_test.go +++ b/runtime/drivers/databricks/olap_test.go @@ -1,6 +1,11 @@ package databricks_test import ( + "context" + "fmt" + "github.com/google/uuid" + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/rilldata/rill/runtime/metricsview" "strings" "testing" "time" @@ -170,6 +175,145 @@ func TestQuerySchema(t *testing.T) { require.Equal(t, "string_col", schema.Fields[1].Name) } +func TestUnnestDimension(t *testing.T) { + testmode.Expensive(t) + _, olap := acquireTestDatabricks(t) + + // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + // The table is created in the DSN's current schema. + name := "test_unnest_" + uuid.New().String()[:8] + t.Cleanup(func() { + err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + name}) + require.NoError(t, err) + }) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT CAST(id AS BIGINT) AS id, tags FROM VALUES (1, array('a', 'b')), (2, array('b')), (3, array('c')), (4, CAST(array() AS ARRAY)) AS t(id, tags)"}) + require.NoError(t, err) + + mv := &runtimev1.MetricsViewSpec{ + Table: name, + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "tags", Column: "tags", Unnest: true}, + {Name: "id", Column: "id"}, + }, + Measures: []*runtimev1.MetricsViewSpec_Measure{ + {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, + }, + } + + // Same query shape as the executor's dimension validation. + dialect := olap.Dialect() + escapeTable := dialect.EscapeTable(mv.Database, mv.DatabaseSchema, mv.Table) + sel, unnestClause, err := dialect.DimensionSelect(escapeTable, mv.Dimensions[0]) + require.NoError(t, err) + err = olap.Exec(t.Context(), &drivers.Statement{Query: fmt.Sprintf("SELECT %s FROM %s %s GROUP BY 1", sel, escapeTable, unnestClause), DryRun: true}) + require.NoError(t, err) + + tagsFilter := func(op metricsview.Operator, val any) *metricsview.Expression { + return &metricsview.Expression{Condition: &metricsview.Condition{ + Operator: op, + Expressions: []*metricsview.Expression{{Name: "tags"}, {Value: val}}, + }} + } + count := func(where *metricsview.Expression) *metricsview.Query { + return &metricsview.Query{Measures: []metricsview.Measure{{Name: "count"}}, Where: where} + } + + tests := []struct { + name string + qry *metricsview.Query + want []map[string]any + }{ + { + name: "group by unnest dimension", + qry: &metricsview.Query{ + Dimensions: []metricsview.Dimension{{Name: "tags"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + Sort: []metricsview.Sort{{Name: "tags"}}, + }, + want: []map[string]any{ + {"tags": "a", "count": int64(1)}, + {"tags": "b", "count": int64(2)}, + {"tags": "c", "count": int64(1)}, + }, + }, + { + // Row 1 matches both values but must be counted once. + name: "in filter counts each source row once", + qry: count(tagsFilter(metricsview.OperatorIn, []any{"a", "b"})), + want: []map[string]any{{"count": int64(2)}}, + }, + { + // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + name: "nin filter excludes rows containing any listed value", + qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), + want: []map[string]any{{"count": int64(3)}}, + }, + { + name: "eq filter", + qry: count(tagsFilter(metricsview.OperatorEq, "b")), + want: []map[string]any{{"count": int64(2)}}, + }, + { + name: "neq filter excludes rows containing the value", + qry: count(tagsFilter(metricsview.OperatorNeq, "b")), + want: []map[string]any{{"count": int64(2)}}, + }, + { + name: "ilike filter", + qry: count(tagsFilter(metricsview.OperatorIlike, "%B%")), + want: []map[string]any{{"count": int64(2)}}, + }, + { + name: "eq filter with no match", + qry: count(tagsFilter(metricsview.OperatorEq, "missing")), + want: []map[string]any{{"count": int64(0)}}, + }, + { + name: "filter combined with group by on another dimension", + qry: &metricsview.Query{ + Dimensions: []metricsview.Dimension{{Name: "id"}}, + Measures: []metricsview.Measure{{Name: "count"}}, + Where: tagsFilter(metricsview.OperatorIn, []any{"a", "b"}), + Sort: []metricsview.Sort{{Name: "id"}}, + }, + want: []map[string]any{ + {"id": int64(1), "count": int64(1)}, + {"id": int64(2), "count": int64(1)}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.qry.MetricsView = mv.Table + ast, err := metricsview.NewAST(mv, allowAllSecurity{}, tt.qry, dialect) + require.NoError(t, err) + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Equal(t, tt.want, queryRows(t, olap, sql, args)) + }) + } +} + +func queryRows(t *testing.T, olap drivers.OLAPStore, query string, args []any) []map[string]any { + rows, err := olap.Query(t.Context(), &drivers.Statement{Query: query, Args: args}) + require.NoError(t, err) + defer rows.Close() + var res []map[string]any + for rows.Next() { + row := make(map[string]any) + require.NoError(t, rows.MapScan(row)) + res = append(res, row) + } + require.NoError(t, rows.Err()) + return res +} + +type allowAllSecurity struct{} + +func (allowAllSecurity) CanAccessField(string) bool { return true } +func (allowAllSecurity) RowFilter() string { return "" } +func (allowAllSecurity) QueryFilter() *runtimev1.Expression { return nil } + func acquireTestDatabricks(t *testing.T) (drivers.Handle, drivers.OLAPStore) { cfg := testruntime.AcquireConnector(t, "databricks") conn, err := drivers.Open("databricks", "", "default", cfg, storage.MustNew(t.TempDir(), nil), activity.NewNoopClient(), zap.NewNop()) diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index 80acd285b969..6bf00a52c4c9 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -48,14 +48,15 @@ type Dialect interface { SupportsRegexMatch() bool GetRegexMatchFunction() (string, error) RequiresArrayContainsForInOperator() bool - GetArrayContainsFunction() (string, error) + // ArrayContainsAnyExpression returns an expression that is true if the array arrExpr contains any of the comma-separated valuesExpr. + ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) // UnnestedColumn returns the expression for the array element exposed by LateralUnnest in tuple style. UnnestedColumn(tableAlias, colName string) string // ArrayAnyExpression returns fragments for a condition that is true if any element of arrExpr satisfies it. // The condition on a single element is written between open and close and references the element as elem. - // ok is false if the dialect has no such expression, in which case a correlated EXISTS subquery over LateralUnnest is used. + // ok is false if the dialect has no such expression, in which case a correlated EXISTS subquery over LateralUnnest is used where possible. ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) UnnestSQLSuffix(tbl string) string // AutoUnnest wraps an expression so the dialect unnests it automatically (used when LateralUnnest reports auto == true). @@ -244,7 +245,7 @@ func (b *BaseDialect) RequiresArrayContainsForInOperator() bool { return false } -func (b *BaseDialect) GetArrayContainsFunction() (string, error) { +func (b *BaseDialect) ArrayContainsAnyExpression(_, _ string) (string, error) { return "", fmt.Errorf("array contains not supported for %s dialect", b.String()) } diff --git a/runtime/drivers/duckdb/dialect.go b/runtime/drivers/duckdb/dialect.go index 3e0cc075ab24..30eea2de3572 100644 --- a/runtime/drivers/duckdb/dialect.go +++ b/runtime/drivers/duckdb/dialect.go @@ -40,7 +40,9 @@ func (d *dialect) EscapeTable(db, schema, table string) string { func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } -func (d *dialect) GetArrayContainsFunction() (string, error) { return "list_has_any", nil } +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { + return fmt.Sprintf("list_has_any(%s, [%s])", arrExpr, valuesExpr), nil +} func (d *dialect) OrderByExpression(name string, desc bool) string { res := d.EscapeIdentifier(name) diff --git a/runtime/drivers/snowflake/dialect.go b/runtime/drivers/snowflake/dialect.go index ba2aec3d39e8..ef7ecce16624 100644 --- a/runtime/drivers/snowflake/dialect.go +++ b/runtime/drivers/snowflake/dialect.go @@ -72,6 +72,12 @@ func (d *dialect) UnnestedColumn(tableAlias, colName string) string { return d.EscapeMember(tableAlias, colName) + "::VARCHAR" } +func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } + +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { + return fmt.Sprintf("ARRAYS_OVERLAP(%s, ARRAY_CONSTRUCT(%s))", arrExpr, valuesExpr), nil +} + // ArrayAnyExpression uses FILTER because Snowflake rejects correlated FLATTEN inside EXISTS subqueries. func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { return fmt.Sprintf("(ARRAY_SIZE(FILTER(%s, %s -> ", arrExpr, elemAlias), elemAlias + "::VARCHAR", ")) > 0)", true diff --git a/runtime/metricsview/ast_unnest_test.go b/runtime/metricsview/ast_unnest_test.go index 88972c894136..2d8dac9b76bf 100644 --- a/runtime/metricsview/ast_unnest_test.go +++ b/runtime/metricsview/ast_unnest_test.go @@ -6,6 +6,7 @@ import ( runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/drivers/bigquery" + "github.com/rilldata/rill/runtime/drivers/databricks" "github.com/rilldata/rill/runtime/drivers/snowflake" "github.com/stretchr/testify/require" ) @@ -40,14 +41,14 @@ func TestUnnestSQL(t *testing.T) { name: "bigquery: group by unnest dim", dialect: bigquery.DialectBigQuery, dims: []Dimension{{Name: "tags"}}, - wantSQL: "SELECT (`t0`.`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(ARRAY(SELECT AS STRUCT `t0_elem` AS `tags` FROM UNNEST(`tags`) AS `t0_elem`)) AS `t0` GROUP BY 1", + wantSQL: "SELECT (`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` GROUP BY 1", }, { name: "bigquery: filter on unnest dim not in select", dialect: bigquery.DialectBigQuery, dims: []Dimension{{Name: "city"}}, where: tagsEqA, - wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE EXISTS (SELECT 1 FROM UNNEST(ARRAY(SELECT AS STRUCT `t0_elem` AS `tags` FROM UNNEST(`tags`) AS `t0_elem`)) AS `t0` WHERE ((`t0`.`tags`) = ?)) GROUP BY 1", + wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE EXISTS (SELECT 1 FROM UNNEST(`tags`) AS `t0` WHERE ((`t0`) = ?)) GROUP BY 1", wantArgs: []any{"a"}, }, { @@ -72,9 +73,23 @@ func TestUnnestSQL(t *testing.T) { Operator: OperatorNin, Expressions: []*Expression{{Name: "tags"}, {Value: []any{"a", "b"}}}, }}, - wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE NOT (ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) IN (?,?)))) > 0) GROUP BY 1`, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE (NOT ARRAYS_OVERLAP((tags), ARRAY_CONSTRUCT(?,?))) GROUP BY 1`, wantArgs: []any{"a", "b"}, }, + { + name: "databricks: group by unnest dim", + dialect: databricks.DialectDatabricks, + dims: []Dimension{{Name: "tags"}}, + wantSQL: "SELECT (`t0`.`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table` LATERAL VIEW EXPLODE(`tags`) t0 AS `tags` GROUP BY 1", + }, + { + name: "databricks: filter on unnest dim not in select", + dialect: databricks.DialectDatabricks, + dims: []Dimension{{Name: "city"}}, + where: tagsEqA, + wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE EXISTS(`tags`, t0 -> ((t0) = ?)) GROUP BY 1", + wantArgs: []any{"a"}, + }, } for _, tt := range tests { diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index 118181f901c2..6c6d1e80b77c 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -277,8 +277,8 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) return b.writeBinaryConditionInner(nil, right, leftExpr, op) } - // For IN/NIN on unnest dimensions backed by DuckDB or ClickHouse, use native array-contains - // functions (list_has_any / hasAny). This avoids double-counting when a row's array contains multiple matching values. + // For IN/NIN on unnest dimensions, prefer a native array-contains expression over an unnest join where the dialect supports it. + // It avoids scanning the unnested rows and double-counting rows whose array contains multiple matching values. if (op == OperatorIn || op == OperatorNin) && b.ast.Dialect.RequiresArrayContainsForInOperator() { return b.writeArrayContainsCondition(leftExpr, right, op == OperatorNin) } @@ -294,16 +294,18 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) leftExpr = b.ast.Dialect.AutoUnnest(leftExpr) return b.writeBinaryConditionInner(nil, right, leftExpr, op) } - var unnestColAlias string - if tupleStyle { - unnestColAlias = b.ast.Dialect.UnnestedColumn(unnestTableAlias, left.Name) - } else { - unnestColAlias = b.ast.Dialect.EscapeAlias(left.Name) - } - - if !tupleStyle { // if tupleStyle, then we cannot refer to the column by table alias + // A filter on an unnest dimension that is not selected should match each source row once, even if several of its elements match. + // Prefer the dialect's native any-element expression, then a correlated EXISTS subquery over the unnest join. + // If the dialect can do neither, fall back to joining the unnest into the outer query, which duplicates rows with several matching elements. + open, elem, closing, ok := b.ast.Dialect.ArrayAnyExpression(leftExpr, unnestTableAlias) + if !ok && !tupleStyle { b.ast.unnests = append(b.ast.unnests, unnestFrom) - return b.writeBinaryConditionInner(nil, right, unnestColAlias, op) + return b.writeBinaryConditionInner(nil, right, b.ast.Dialect.EscapeAlias(left.Name), op) + } + if !ok { + open = "EXISTS (SELECT 1 FROM " + unnestFrom + " WHERE " + elem = b.ast.Dialect.UnnestedColumn(unnestTableAlias, left.Name) + closing = ")" } // Need to move "NOT" to outside of the subquery @@ -320,17 +322,9 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) not = true } - // Evaluate the condition per source row so a row matches once even if several of its elements match. - // Output: [NOT] EXISTS (SELECT 1 FROM WHERE ), unless the dialect has a native array expression. if not { b.writeString("NOT ") } - open, elem, closing, ok := b.ast.Dialect.ArrayAnyExpression(leftExpr, unnestTableAlias) - if !ok { - open = "EXISTS (SELECT 1 FROM " + unnestFrom + " WHERE " - elem = unnestColAlias - closing = ")" - } b.writeString(open) err = b.writeBinaryConditionInner(nil, right, elem, op) if err != nil { @@ -683,24 +677,14 @@ func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, right *Exp if not { b.writeString("NOT ") } - arrayContainsFunc, err := b.ast.Dialect.GetArrayContainsFunction() + // NULL values in the list are not handled separately: ClickHouse's hasAny and Snowflake's ARRAYS_OVERLAP match them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. + // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. + b.args = append(b.args, vals...) + expr, err := b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) if err != nil { return err } - b.writeString(arrayContainsFunc) - b.writeByte('(') - b.writeParenthesizedString(leftExpr) - b.writeString(", [") - // not handling NULL values separately as clickhouse hasAny function takes care of it however, duckdb ignores null values in the list_has_any function, but there is no reliable way to make it work, - // but even using leftExpr IS NULL does not solve the issue as it checks for null array rather than null values in the array. - for i, val := range vals { - if i > 0 { - b.writeByte(',') - } - b.writeString("?") - b.args = append(b.args, val) - } - b.writeString("])") + b.writeString(expr) b.writeByte(')') return nil diff --git a/runtime/metricsview/astexpr_test.go b/runtime/metricsview/astexpr_test.go index 5abe4b87c431..9612b7fc438a 100644 --- a/runtime/metricsview/astexpr_test.go +++ b/runtime/metricsview/astexpr_test.go @@ -6,7 +6,9 @@ import ( runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/drivers/clickhouse" + "github.com/rilldata/rill/runtime/drivers/databricks" "github.com/rilldata/rill/runtime/drivers/duckdb" + "github.com/rilldata/rill/runtime/drivers/snowflake" "github.com/stretchr/testify/require" ) @@ -138,6 +140,59 @@ func TestArrayContainsCondition(t *testing.T) { wantSQL: `(hasAny(("tags"), [?,?]))`, wantArgs: []any{nil, "a"}, }, + { + name: "databricks: in on unnest dim uses arrays_overlap", + dialect: databricks.DialectDatabricks, + where: &Expression{Condition: &Condition{ + Operator: OperatorIn, + Expressions: []*Expression{ + {Name: "tags"}, + {Value: []any{"a", "b"}}, + }, + }}, + wantSQL: "(arrays_overlap((`tags`), array(?,?)))", + wantArgs: []any{"a", "b"}, + }, + { + name: "databricks: nin on unnest dim uses NOT arrays_overlap", + dialect: databricks.DialectDatabricks, + where: &Expression{Condition: &Condition{ + Operator: OperatorNin, + Expressions: []*Expression{ + {Name: "tags"}, + {Value: []any{"a", "b"}}, + }, + }}, + wantSQL: "(NOT arrays_overlap((`tags`), array(?,?)))", + wantArgs: []any{"a", "b"}, + }, + { + name: "databricks: in on unnest dim already in select falls back to normal IN", + dialect: databricks.DialectDatabricks, + dims: []Dimension{{Name: "tags"}}, + where: &Expression{Condition: &Condition{ + Operator: OperatorIn, + Expressions: []*Expression{ + {Name: "tags"}, + {Value: []any{"a", "b"}}, + }, + }}, + wantSQL: "((`t0`.`tags`) IN (?,?))", + wantArgs: []any{"a", "b"}, + }, + { + name: "snowflake: in on unnest dim uses ARRAYS_OVERLAP", + dialect: snowflake.DialectSnowflake, + where: &Expression{Condition: &Condition{ + Operator: OperatorIn, + Expressions: []*Expression{ + {Name: "tags"}, + {Value: []any{"a", "b"}}, + }, + }}, + wantSQL: "(ARRAYS_OVERLAP((tags), ARRAY_CONSTRUCT(?,?)))", + wantArgs: []any{"a", "b"}, + }, { name: "duckdb: in on non-unnest dim uses normal IN", dialect: duckdb.DialectDuckDB, From 7b9323112ce298c22d905533fbc8ea00d78bada8 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:12:37 +0530 Subject: [PATCH 4/8] skip databricks and snowflake tests --- runtime/drivers/databricks/olap_test.go | 1 + runtime/drivers/snowflake/olap_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/runtime/drivers/databricks/olap_test.go b/runtime/drivers/databricks/olap_test.go index 840566410dfe..503c1899d15c 100644 --- a/runtime/drivers/databricks/olap_test.go +++ b/runtime/drivers/databricks/olap_test.go @@ -176,6 +176,7 @@ func TestQuerySchema(t *testing.T) { } func TestUnnestDimension(t *testing.T) { + t.Skip("skipping due to inactive Databricks account") testmode.Expensive(t) _, olap := acquireTestDatabricks(t) diff --git a/runtime/drivers/snowflake/olap_test.go b/runtime/drivers/snowflake/olap_test.go index 0fdb96b81258..2071c59140e8 100644 --- a/runtime/drivers/snowflake/olap_test.go +++ b/runtime/drivers/snowflake/olap_test.go @@ -191,6 +191,7 @@ func TestDryRun(t *testing.T) { // TestUnnestDimension creates a table in the DSN's current database and schema, so the DSN must point at a writable schema. func TestUnnestDimension(t *testing.T) { + t.Skip("skipping due to inactive Snowflake account") testmode.Expensive(t) _, olap := acquireTestSnowflake(t) From 768edaf2f137415d1f7771155b183d55667a96c7 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:43:17 +0530 Subject: [PATCH 5/8] more text fixes --- runtime/drivers/bigquery/dialect.go | 7 +++ runtime/drivers/bigquery/olap_test.go | 27 +++++++++-- runtime/drivers/databricks/dialect.go | 12 ++++- runtime/drivers/databricks/olap_test.go | 30 +++++++++--- runtime/drivers/dialect.go | 7 +++ runtime/drivers/snowflake/dialect.go | 12 +++-- runtime/drivers/snowflake/olap_test.go | 30 +++++++++--- runtime/metricsview/ast_unnest_test.go | 63 +++++++++++++++++++++++-- runtime/metricsview/astexpr.go | 59 +++++++++++++++-------- runtime/metricsview/astexpr_test.go | 8 ++-- 10 files changed, 205 insertions(+), 50 deletions(-) diff --git a/runtime/drivers/bigquery/dialect.go b/runtime/drivers/bigquery/dialect.go index 18171fde60fc..d7e51506421a 100644 --- a/runtime/drivers/bigquery/dialect.go +++ b/runtime/drivers/bigquery/dialect.go @@ -88,6 +88,13 @@ func (d *dialect) LateralUnnest(expr, _, colName string) (tbl string, tupleStyle return fmt.Sprintf(`UNNEST(%s) AS %s`, expr, d.EscapeIdentifier(colName)), false, false, nil } +// ArrayContainsSubqueryExpression joins the subquery to the unnested array. +// BigQuery cannot de-correlate an IN subquery that references another table inside a correlated EXISTS. +// UNNEST comes first so that the array expression resolves against the outer query and cannot be shadowed by the subquery's column. +func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { + return fmt.Sprintf("EXISTS (SELECT 1 FROM UNNEST(%s) AS e JOIN %s AS s ON e = s.%s)", arrExpr, subquerySQL, valueCol), true +} + func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { elem = d.EscapeIdentifier(elemAlias) return fmt.Sprintf("EXISTS (SELECT 1 FROM UNNEST(%s) AS %s WHERE ", arrExpr, elem), elem, ")", true diff --git a/runtime/drivers/bigquery/olap_test.go b/runtime/drivers/bigquery/olap_test.go index 8d1d1f0c98f4..cfccc50604dc 100644 --- a/runtime/drivers/bigquery/olap_test.go +++ b/runtime/drivers/bigquery/olap_test.go @@ -127,14 +127,15 @@ func TestUnnestDimension(t *testing.T) { testmode.Expensive(t) _, olap := acquireTestBigQuery(t) - // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + // Rows with overlapping, empty and NULL arrays, so joins that duplicate source rows and NULL handling are detectable. + // BigQuery arrays cannot contain NULL elements, and a NULL array is stored as an empty array. name := "test_unnest_" + uuid.New().String()[:8] table := "`rilldata.integration_test." + name + "`" t.Cleanup(func() { err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + table}) require.NoError(t, err) }) - err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + table + " AS SELECT id, tags FROM UNNEST(ARRAY>>[(1, ['a', 'b']), (2, ['b']), (3, ['c']), (4, ARRAY[])])"}) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + table + " AS SELECT id, tags FROM UNNEST(ARRAY>>[(1, ['a', 'b']), (2, ['b']), (3, ['c']), (4, ARRAY[]), (5, NULL)])"}) require.NoError(t, err) mv := &runtimev1.MetricsViewSpec{ @@ -193,10 +194,10 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{{"count": int64(2)}}, }, { - // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + // Excludes rows containing 'a' even if they also contain other values; keeps the empty and NULL arrays. name: "nin filter excludes rows containing any listed value", qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), - want: []map[string]any{{"count": int64(3)}}, + want: []map[string]any{{"count": int64(4)}}, }, { name: "eq filter", @@ -206,13 +207,29 @@ func TestUnnestDimension(t *testing.T) { { name: "neq filter excludes rows containing the value", qry: count(tagsFilter(metricsview.OperatorNeq, "b")), - want: []map[string]any{{"count": int64(2)}}, + want: []map[string]any{{"count": int64(3)}}, }, { name: "eq filter with no match", qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": int64(0)}}, }, + { + // Measure filter: the only dimension value with more than one row is 'b'. + name: "in filter with measure-filter subquery", + qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ + Operator: metricsview.OperatorIn, + Expressions: []*metricsview.Expression{ + {Name: "tags"}, + {Subquery: &metricsview.Subquery{ + Dimension: metricsview.Dimension{Name: "tags"}, + Measures: []metricsview.Measure{{Name: "count"}}, + Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, + }}, + }, + }}), + want: []map[string]any{{"count": int64(2)}}, + }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/drivers/databricks/dialect.go b/runtime/drivers/databricks/dialect.go index 5c320ecf7d91..580fbcb7b105 100644 --- a/runtime/drivers/databricks/dialect.go +++ b/runtime/drivers/databricks/dialect.go @@ -80,14 +80,22 @@ func (d *dialect) UnnestSQLSuffix(tbl string) string { return fmt.Sprintf(" %s", tbl) } +// ArrayAnyExpression wraps EXISTS in COALESCE: it returns NULL rather than false when no element matches and some element is NULL, +// which would otherwise make negated filters drop the row. func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { - return fmt.Sprintf("EXISTS(%s, %s -> ", arrExpr, elemAlias), elemAlias, ")", true + return fmt.Sprintf("COALESCE(EXISTS(%s, %s -> ", arrExpr, elemAlias), elemAlias, "), FALSE)", true } func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } +// ArrayContainsAnyExpression wraps arrays_overlap in COALESCE for the same reason as ArrayAnyExpression. func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { - return fmt.Sprintf("arrays_overlap(%s, array(%s))", arrExpr, valuesExpr), nil + return fmt.Sprintf("COALESCE(arrays_overlap(%s, array(%s)), FALSE)", arrExpr, valuesExpr), nil +} + +// ArrayContainsSubqueryExpression collects the subquery into an array because Databricks does not allow subqueries inside lambda functions. +func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { + return fmt.Sprintf("COALESCE(arrays_overlap(%s, (SELECT collect_list(s.%s) FROM %s AS s)), FALSE)", arrExpr, valueCol, subquerySQL), true } func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { diff --git a/runtime/drivers/databricks/olap_test.go b/runtime/drivers/databricks/olap_test.go index 503c1899d15c..a9ac239dd901 100644 --- a/runtime/drivers/databricks/olap_test.go +++ b/runtime/drivers/databricks/olap_test.go @@ -180,14 +180,14 @@ func TestUnnestDimension(t *testing.T) { testmode.Expensive(t) _, olap := acquireTestDatabricks(t) - // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + // Rows with overlapping, empty, NULL-element and NULL arrays, so joins that duplicate source rows and NULL handling are detectable. // The table is created in the DSN's current schema. name := "test_unnest_" + uuid.New().String()[:8] t.Cleanup(func() { err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + name}) require.NoError(t, err) }) - err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT CAST(id AS BIGINT) AS id, tags FROM VALUES (1, array('a', 'b')), (2, array('b')), (3, array('c')), (4, CAST(array() AS ARRAY)) AS t(id, tags)"}) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT CAST(id AS BIGINT) AS id, tags FROM VALUES (1, array('a', 'b')), (2, array('b')), (3, array('c')), (4, CAST(array() AS ARRAY)), (5, array('c', NULL)), (6, CAST(NULL AS ARRAY)) AS t(id, tags)"}) require.NoError(t, err) mv := &runtimev1.MetricsViewSpec{ @@ -234,7 +234,8 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{ {"tags": "a", "count": int64(1)}, {"tags": "b", "count": int64(2)}, - {"tags": "c", "count": int64(1)}, + {"tags": "c", "count": int64(2)}, + {"tags": nil, "count": int64(1)}, }, }, { @@ -244,10 +245,10 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{{"count": int64(2)}}, }, { - // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + // Excludes rows containing 'a' even if they also contain other values; keeps the empty, NULL-element and NULL arrays. name: "nin filter excludes rows containing any listed value", qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), - want: []map[string]any{{"count": int64(3)}}, + want: []map[string]any{{"count": int64(5)}}, }, { name: "eq filter", @@ -255,9 +256,10 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{{"count": int64(2)}}, }, { + // Keeps the NULL-element and NULL arrays: a NULL comparison must not be treated as a match. name: "neq filter excludes rows containing the value", qry: count(tagsFilter(metricsview.OperatorNeq, "b")), - want: []map[string]any{{"count": int64(2)}}, + want: []map[string]any{{"count": int64(4)}}, }, { name: "ilike filter", @@ -269,6 +271,22 @@ func TestUnnestDimension(t *testing.T) { qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": int64(0)}}, }, + { + // Measure filter: dimension values with more than one row are 'b' and 'c'. + name: "in filter with measure-filter subquery", + qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ + Operator: metricsview.OperatorIn, + Expressions: []*metricsview.Expression{ + {Name: "tags"}, + {Subquery: &metricsview.Subquery{ + Dimension: metricsview.Dimension{Name: "tags"}, + Measures: []metricsview.Measure{{Name: "count"}}, + Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, + }}, + }, + }}), + want: []map[string]any{{"count": int64(4)}}, + }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index 6bf00a52c4c9..c13d59bc2359 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -50,6 +50,9 @@ type Dialect interface { RequiresArrayContainsForInOperator() bool // ArrayContainsAnyExpression returns an expression that is true if the array arrExpr contains any of the comma-separated valuesExpr. ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) + // ArrayContainsSubqueryExpression is like ArrayContainsAnyExpression but takes a parenthesized subquery whose values are in the column valueCol. + // ok is false if the dialect has no such expression, in which case the condition is evaluated against the unnested elements instead. + ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) // UnnestedColumn returns the expression for the array element exposed by LateralUnnest in tuple style. @@ -249,6 +252,10 @@ func (b *BaseDialect) ArrayContainsAnyExpression(_, _ string) (string, error) { return "", fmt.Errorf("array contains not supported for %s dialect", b.String()) } +func (b *BaseDialect) ArrayContainsSubqueryExpression(_, _, _ string) (expr string, ok bool) { + return "", false +} + func (b *BaseDialect) MetricsViewDimensionExpression(dimension *runtimev1.MetricsViewSpec_Dimension) (string, error) { if dimension.LookupTable != "" { return "", fmt.Errorf("lookup tables are not supported for %s dialect", b.String()) diff --git a/runtime/drivers/snowflake/dialect.go b/runtime/drivers/snowflake/dialect.go index ef7ecce16624..6df37fb0c3dc 100644 --- a/runtime/drivers/snowflake/dialect.go +++ b/runtime/drivers/snowflake/dialect.go @@ -72,15 +72,17 @@ func (d *dialect) UnnestedColumn(tableAlias, colName string) string { return d.EscapeMember(tableAlias, colName) + "::VARCHAR" } -func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } - -func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { - return fmt.Sprintf("ARRAYS_OVERLAP(%s, ARRAY_CONSTRUCT(%s))", arrExpr, valuesExpr), nil +// ArrayContainsSubqueryExpression aggregates the subquery into an array and tests each element against it. +// An IN subquery inside the FILTER lambda causes an internal error in Snowflake, and ARRAYS_OVERLAP does not accept structured arrays. +func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { + return fmt.Sprintf("COALESCE(ARRAY_SIZE(FILTER(%s, x -> ARRAY_CONTAINS(x::VARCHAR::VARIANT, (SELECT ARRAY_AGG(s.%s) FROM %s AS s)))) > 0, FALSE)", arrExpr, valueCol, subquerySQL), true } // ArrayAnyExpression uses FILTER because Snowflake rejects correlated FLATTEN inside EXISTS subqueries. +// It also serves IN filters: ARRAYS_OVERLAP does not accept structured arrays and compares raw VARIANT elements, which would not match the VARCHAR values shown by UnnestedColumn. +// FILTER(NULL, ...) is NULL, so the result is coalesced to FALSE to keep rows with a NULL array under negated filters. func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { - return fmt.Sprintf("(ARRAY_SIZE(FILTER(%s, %s -> ", arrExpr, elemAlias), elemAlias + "::VARCHAR", ")) > 0)", true + return fmt.Sprintf("COALESCE(ARRAY_SIZE(FILTER(%s, %s -> ", arrExpr, elemAlias), elemAlias + "::VARCHAR", ")) > 0, FALSE)", true } func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { diff --git a/runtime/drivers/snowflake/olap_test.go b/runtime/drivers/snowflake/olap_test.go index 2071c59140e8..c275a5e95da4 100644 --- a/runtime/drivers/snowflake/olap_test.go +++ b/runtime/drivers/snowflake/olap_test.go @@ -195,14 +195,14 @@ func TestUnnestDimension(t *testing.T) { testmode.Expensive(t) _, olap := acquireTestSnowflake(t) - // Rows with overlapping and empty arrays, so joins that duplicate source rows are detectable. + // Rows with overlapping, empty, NULL-element and NULL arrays, so joins that duplicate source rows and NULL handling are detectable. // The driver returns NUMBER columns as strings. name := "test_unnest_" + uuid.New().String()[:8] t.Cleanup(func() { err := olap.Exec(context.Background(), &drivers.Statement{Query: "DROP TABLE IF EXISTS " + name}) require.NoError(t, err) }) - err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT 1 AS id, ARRAY_CONSTRUCT('a', 'b') AS tags UNION ALL SELECT 2, ARRAY_CONSTRUCT('b') UNION ALL SELECT 3, ARRAY_CONSTRUCT('c') UNION ALL SELECT 4, ARRAY_CONSTRUCT()"}) + err := olap.Exec(t.Context(), &drivers.Statement{Query: "CREATE TABLE " + name + " AS SELECT 1 AS id, ARRAY_CONSTRUCT('a', 'b') AS tags UNION ALL SELECT 2, ARRAY_CONSTRUCT('b') UNION ALL SELECT 3, ARRAY_CONSTRUCT('c') UNION ALL SELECT 4, ARRAY_CONSTRUCT() UNION ALL SELECT 5, ARRAY_CONSTRUCT('c', NULL) UNION ALL SELECT 6, NULL::ARRAY"}) require.NoError(t, err) mv := &runtimev1.MetricsViewSpec{ @@ -249,7 +249,8 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{ {"tags": "a", "count": "1"}, {"tags": "b", "count": "2"}, - {"tags": "c", "count": "1"}, + // FLATTEN does not emit a row for a NULL element. + {"tags": "c", "count": "2"}, }, }, { @@ -259,10 +260,10 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{{"count": "2"}}, }, { - // Excludes rows containing 'a' even if they also contain other values; keeps the empty array. + // Excludes rows containing 'a' even if they also contain other values; keeps the empty, NULL-element and NULL arrays. name: "nin filter excludes rows containing any listed value", qry: count(tagsFilter(metricsview.OperatorNin, []any{"a"})), - want: []map[string]any{{"count": "3"}}, + want: []map[string]any{{"count": "5"}}, }, { name: "eq filter", @@ -270,9 +271,10 @@ func TestUnnestDimension(t *testing.T) { want: []map[string]any{{"count": "2"}}, }, { + // Keeps the NULL-element and NULL arrays: a NULL comparison must not be treated as a match. name: "neq filter excludes rows containing the value", qry: count(tagsFilter(metricsview.OperatorNeq, "b")), - want: []map[string]any{{"count": "2"}}, + want: []map[string]any{{"count": "4"}}, }, { name: "ilike filter", @@ -284,6 +286,22 @@ func TestUnnestDimension(t *testing.T) { qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": "0"}}, }, + { + // Measure filter: dimension values with more than one row are 'b' and 'c'. + name: "in filter with measure-filter subquery", + qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ + Operator: metricsview.OperatorIn, + Expressions: []*metricsview.Expression{ + {Name: "tags"}, + {Subquery: &metricsview.Subquery{ + Dimension: metricsview.Dimension{Name: "tags"}, + Measures: []metricsview.Measure{{Name: "count"}}, + Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, + }}, + }, + }}), + want: []map[string]any{{"count": "4"}}, + }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/metricsview/ast_unnest_test.go b/runtime/metricsview/ast_unnest_test.go index 2d8dac9b76bf..c2ff1ecaf568 100644 --- a/runtime/metricsview/ast_unnest_test.go +++ b/runtime/metricsview/ast_unnest_test.go @@ -7,6 +7,7 @@ import ( "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/drivers/bigquery" "github.com/rilldata/rill/runtime/drivers/databricks" + "github.com/rilldata/rill/runtime/drivers/duckdb" "github.com/rilldata/rill/runtime/drivers/snowflake" "github.com/stretchr/testify/require" ) @@ -62,7 +63,7 @@ func TestUnnestSQL(t *testing.T) { dialect: snowflake.DialectSnowflake, dims: []Dimension{{Name: "city"}}, where: tagsEqA, - wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE (ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) = ?))) > 0) GROUP BY 1`, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE COALESCE(ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) = ?))) > 0, FALSE) GROUP BY 1`, wantArgs: []any{"a"}, }, { @@ -73,7 +74,7 @@ func TestUnnestSQL(t *testing.T) { Operator: OperatorNin, Expressions: []*Expression{{Name: "tags"}, {Value: []any{"a", "b"}}}, }}, - wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE (NOT ARRAYS_OVERLAP((tags), ARRAY_CONSTRUCT(?,?))) GROUP BY 1`, + wantSQL: `SELECT (city) AS "city", (count(*)) AS "count" FROM test_table WHERE NOT COALESCE(ARRAY_SIZE(FILTER(tags, t0 -> ((t0::VARCHAR) IN (?,?)))) > 0, FALSE) GROUP BY 1`, wantArgs: []any{"a", "b"}, }, { @@ -87,7 +88,7 @@ func TestUnnestSQL(t *testing.T) { dialect: databricks.DialectDatabricks, dims: []Dimension{{Name: "city"}}, where: tagsEqA, - wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE EXISTS(`tags`, t0 -> ((t0) = ?)) GROUP BY 1", + wantSQL: "SELECT (`city`) AS `city`, (count(*)) AS `count` FROM `test_table` WHERE COALESCE(EXISTS(`tags`, t0 -> ((t0) = ?)), FALSE) GROUP BY 1", wantArgs: []any{"a"}, }, } @@ -111,3 +112,59 @@ func TestUnnestSQL(t *testing.T) { }) } } + +// Measure filters produce "dim IN (subquery)". Dialects with an array-contains fast path must still handle them. +func TestUnnestSubqueryFilterSQL(t *testing.T) { + mv := &runtimev1.MetricsViewSpec{ + Table: "test_table", + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "tags", Column: "tags", Unnest: true}, + {Name: "city", Column: "city"}, + }, + Measures: []*runtimev1.MetricsViewSpec_Measure{ + {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, + }, + } + where := &Expression{Condition: &Condition{ + Operator: OperatorNin, + Expressions: []*Expression{ + {Name: "tags"}, + {Subquery: &Subquery{ + Dimension: Dimension{Name: "tags"}, + Measures: []Measure{{Name: "count"}}, + Having: &Expression{Condition: &Condition{Operator: OperatorGt, Expressions: []*Expression{{Name: "count"}, {Value: 10}}}}, + }}, + }, + }} + // The subquery is the metrics view grouped by the unnest dimension, with the having clause applied in an outer select. + sub := map[string]string{ + "duckdb": `(SELECT "tags" FROM (SELECT ("t2"."tags") AS "tags", ("t2"."count") AS "count" FROM (SELECT ("t0"."tags") AS "tags", (count(*)) AS "count" FROM "test_table", LATERAL UNNEST("tags") t0("tags") GROUP BY 1) t2 WHERE (("t2"."count") > ?)))`, + "databricks": "(SELECT `tags` FROM (SELECT (`t2`.`tags`) AS `tags`, (`t2`.`count`) AS `count` FROM (SELECT (`t0`.`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table` LATERAL VIEW EXPLODE(`tags`) t0 AS `tags` GROUP BY 1) t2 WHERE ((`t2`.`count`) > ?)))", + "snowflake": `(SELECT "tags" FROM (SELECT (t2."tags") AS "tags", (t2."count") AS "count" FROM (SELECT (t0.tags::VARCHAR) AS "tags", (count(*)) AS "count" FROM test_table, LATERAL FLATTEN(INPUT => tags) t0 (seq, key, path, index, tags, this) GROUP BY 1) t2 WHERE ((t2."count") > ?)))`, + "bigquery": "(SELECT `tags` FROM (SELECT (`t2`.`tags`) AS `tags`, (`t2`.`count`) AS `count` FROM (SELECT (`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` GROUP BY 1) t2 WHERE ((`t2`.`count`) > ?)))", + } + tests := []struct { + dialect drivers.Dialect + want string + }{ + // No native form: correlated EXISTS over the unnest join. + {duckdb.DialectDuckDB, `WHERE NOT EXISTS (SELECT 1 FROM LATERAL UNNEST("tags") t0("tags") WHERE (("t0"."tags") IN ` + sub["duckdb"] + `)) GROUP BY 1`}, + // Lambdas cannot contain subqueries: aggregate the subquery into an array. + {databricks.DialectDatabricks, "WHERE (NOT COALESCE(arrays_overlap((`tags`), (SELECT collect_list(s.`tags`) FROM " + sub["databricks"] + " AS s)), FALSE)) GROUP BY 1"}, + // IN subquery inside FILTER hits an internal error: aggregate into an array and use ARRAY_CONTAINS. + {snowflake.DialectSnowflake, `WHERE (NOT COALESCE(ARRAY_SIZE(FILTER((tags), x -> ARRAY_CONTAINS(x::VARCHAR::VARIANT, (SELECT ARRAY_AGG(s."tags") FROM ` + sub["snowflake"] + ` AS s)))) > 0, FALSE)) GROUP BY 1`}, + // A table-referencing subquery inside correlated EXISTS cannot be de-correlated: join the subquery to the unnested array instead. + {bigquery.DialectBigQuery, "WHERE (NOT EXISTS (SELECT 1 FROM UNNEST((`tags`)) AS e JOIN " + sub["bigquery"] + " AS s ON e = s.`tags`)) GROUP BY 1"}, + } + for _, tt := range tests { + t.Run(tt.dialect.String(), func(t *testing.T) { + qry := &Query{MetricsView: "test", Dimensions: []Dimension{{Name: "city"}}, Measures: []Measure{{Name: "count"}}, Where: where} + ast, err := NewAST(mv, skipMetricsViewSecurity{}, qry, tt.dialect) + require.NoError(t, err) + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Contains(t, sql, tt.want) + require.Equal(t, []any{10}, args) + }) + } +} diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index 6c6d1e80b77c..488b98493743 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -81,6 +81,17 @@ func (b *sqlExprBuilder) writeValue(val any) error { } func (b *sqlExprBuilder) writeSubquery(sub *Subquery) error { + sql, err := b.subquerySQL(sub) + if err != nil { + return err + } + b.writeString(sql) + return nil +} + +// subquerySQL returns "(SELECT FROM ())" and appends the subquery's args to b.args. +// The caller must write the returned SQL before writing any further args. +func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, error) { // We construct a Query that combines the parent Query's contextual info with that of the Subquery. outer := b.ast.Query inner := &Query{ @@ -110,21 +121,15 @@ func (b *sqlExprBuilder) writeSubquery(sub *Subquery) error { } innerAST, err := NewAST(b.ast.MetricsView, innerSecurity, inner, b.ast.Dialect) if err != nil { - return fmt.Errorf("failed to create AST for subquery: %w", err) + return "", fmt.Errorf("failed to create AST for subquery: %w", err) } sql, args, err := innerAST.SQL() if err != nil { - return fmt.Errorf("failed to generate SQL for subquery: %w", err) + return "", fmt.Errorf("failed to generate SQL for subquery: %w", err) } - - // Output: (SELECT FROM ()) - b.writeString("(SELECT ") - b.writeString(b.ast.Dialect.EscapeIdentifier(sub.Dimension.Name)) - b.writeString(" FROM (") - b.writeString(sql) - b.writeString("))") b.args = append(b.args, args...) - return nil + // The dimension is selected by its alias, which some dialects (e.g. Snowflake) escape differently from identifiers. + return fmt.Sprintf("(SELECT %s FROM (%s))", b.ast.Dialect.EscapeAlias(sub.Dimension.Name), sql), nil } func (b *sqlExprBuilder) writeCondition(cond *Condition) error { @@ -279,8 +284,29 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) // For IN/NIN on unnest dimensions, prefer a native array-contains expression over an unnest join where the dialect supports it. // It avoids scanning the unnested rows and double-counting rows whose array contains multiple matching values. - if (op == OperatorIn || op == OperatorNin) && b.ast.Dialect.RequiresArrayContainsForInOperator() { - return b.writeArrayContainsCondition(leftExpr, right, op == OperatorNin) + // Subqueries (e.g. from measure filters) only take this path if the dialect can consume them; otherwise they are evaluated against the unnested elements below. + if op == OperatorIn || op == OperatorNin { + if vals, ok := right.Value.([]any); ok && b.ast.Dialect.RequiresArrayContainsForInOperator() { + return b.writeArrayContainsCondition(leftExpr, vals, op == OperatorNin) + } + if right.Subquery != nil { + nargs := len(b.args) + sql, err := b.subquerySQL(right.Subquery) + if err != nil { + return err + } + if expr, ok := b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)); ok { + b.writeByte('(') + if op == OperatorNin { + b.writeString("NOT ") + } + b.writeString(expr) + b.writeByte(')') + return nil + } + // The dialect did not use the subquery; drop its args since it is rendered again below. + b.args = b.args[:nargs] + } } // Generate unnest join @@ -658,12 +684,7 @@ func (b *sqlExprBuilder) writeInConditionForValues(left *Expression, leftOverrid return nil } -func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, right *Expression, not bool) error { - vals, ok := right.Value.([]any) - if !ok { - return fmt.Errorf("the right value must be a list of values for an array IN condition") - } - +func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, vals []any, not bool) error { if len(vals) == 0 { if not { b.writeString("TRUE") @@ -677,7 +698,7 @@ func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, right *Exp if not { b.writeString("NOT ") } - // NULL values in the list are not handled separately: ClickHouse's hasAny and Snowflake's ARRAYS_OVERLAP match them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. + // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. b.args = append(b.args, vals...) expr, err := b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) diff --git a/runtime/metricsview/astexpr_test.go b/runtime/metricsview/astexpr_test.go index 9612b7fc438a..2ef354ed5758 100644 --- a/runtime/metricsview/astexpr_test.go +++ b/runtime/metricsview/astexpr_test.go @@ -150,7 +150,7 @@ func TestArrayContainsCondition(t *testing.T) { {Value: []any{"a", "b"}}, }, }}, - wantSQL: "(arrays_overlap((`tags`), array(?,?)))", + wantSQL: "(COALESCE(arrays_overlap((`tags`), array(?,?)), FALSE))", wantArgs: []any{"a", "b"}, }, { @@ -163,7 +163,7 @@ func TestArrayContainsCondition(t *testing.T) { {Value: []any{"a", "b"}}, }, }}, - wantSQL: "(NOT arrays_overlap((`tags`), array(?,?)))", + wantSQL: "(NOT COALESCE(arrays_overlap((`tags`), array(?,?)), FALSE))", wantArgs: []any{"a", "b"}, }, { @@ -181,7 +181,7 @@ func TestArrayContainsCondition(t *testing.T) { wantArgs: []any{"a", "b"}, }, { - name: "snowflake: in on unnest dim uses ARRAYS_OVERLAP", + name: "snowflake: in on unnest dim uses FILTER", dialect: snowflake.DialectSnowflake, where: &Expression{Condition: &Condition{ Operator: OperatorIn, @@ -190,7 +190,7 @@ func TestArrayContainsCondition(t *testing.T) { {Value: []any{"a", "b"}}, }, }}, - wantSQL: "(ARRAYS_OVERLAP((tags), ARRAY_CONSTRUCT(?,?)))", + wantSQL: "COALESCE(ARRAY_SIZE(FILTER(tags, t2 -> ((t2::VARCHAR) IN (?,?)))) > 0, FALSE)", wantArgs: []any{"a", "b"}, }, { From 5302215713e3e5d581fe601e76d48e4ee4bd447b Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Tue, 15 Sep 2026 16:10:48 +0530 Subject: [PATCH 6/8] simplifications --- runtime/drivers/clickhouse/dialect.go | 6 +- runtime/drivers/databricks/dialect.go | 6 +- runtime/drivers/dialect.go | 15 ++--- runtime/drivers/duckdb/dialect.go | 6 +- runtime/metricsview/astexpr.go | 94 +++++++++++---------------- 5 files changed, 51 insertions(+), 76 deletions(-) diff --git a/runtime/drivers/clickhouse/dialect.go b/runtime/drivers/clickhouse/dialect.go index e6c3bad22653..42f418040707 100644 --- a/runtime/drivers/clickhouse/dialect.go +++ b/runtime/drivers/clickhouse/dialect.go @@ -80,10 +80,8 @@ func (d *dialect) AutoUnnest(expr string) string { return fmt.Sprintf("arrayJoin(%s)", expr) } -func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } - -func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { - return fmt.Sprintf("hasAny(%s, [%s])", arrExpr, valuesExpr), nil +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr string, ok bool) { + return fmt.Sprintf("hasAny(%s, [%s])", arrExpr, valuesExpr), true } func (d *dialect) CastToDataType(typ runtimev1.Type_Code) (string, error) { diff --git a/runtime/drivers/databricks/dialect.go b/runtime/drivers/databricks/dialect.go index 580fbcb7b105..c099bfafcc38 100644 --- a/runtime/drivers/databricks/dialect.go +++ b/runtime/drivers/databricks/dialect.go @@ -86,11 +86,9 @@ func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, clo return fmt.Sprintf("COALESCE(EXISTS(%s, %s -> ", arrExpr, elemAlias), elemAlias, "), FALSE)", true } -func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } - // ArrayContainsAnyExpression wraps arrays_overlap in COALESCE for the same reason as ArrayAnyExpression. -func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { - return fmt.Sprintf("COALESCE(arrays_overlap(%s, array(%s)), FALSE)", arrExpr, valuesExpr), nil +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr string, ok bool) { + return fmt.Sprintf("COALESCE(arrays_overlap(%s, array(%s)), FALSE)", arrExpr, valuesExpr), true } // ArrayContainsSubqueryExpression collects the subquery into an array because Databricks does not allow subqueries inside lambda functions. diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index c13d59bc2359..8588f97807ea 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -47,13 +47,14 @@ type Dialect interface { GetCastExprForLike() string SupportsRegexMatch() bool GetRegexMatchFunction() (string, error) - RequiresArrayContainsForInOperator() bool // ArrayContainsAnyExpression returns an expression that is true if the array arrExpr contains any of the comma-separated valuesExpr. - ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) - // ArrayContainsSubqueryExpression is like ArrayContainsAnyExpression but takes a parenthesized subquery whose values are in the column valueCol. // ok is false if the dialect has no such expression, in which case the condition is evaluated against the unnested elements instead. + ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr string, ok bool) + // ArrayContainsSubqueryExpression is like ArrayContainsAnyExpression but takes a parenthesized subquery whose values are in the column valueCol. ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) + // LateralUnnest returns the join clause that unnests expr. If tupleStyle is false the element is referenced by colName alone, + // and the dialect must implement ArrayAnyExpression since it cannot be referenced from a correlated subquery. LateralUnnest(expr, tableAlias, colName string) (tbl string, tupleStyle, auto bool, err error) // UnnestedColumn returns the expression for the array element exposed by LateralUnnest in tuple style. UnnestedColumn(tableAlias, colName string) string @@ -244,12 +245,8 @@ func (b *BaseDialect) AutoUnnest(expr string) string { return expr } -func (b *BaseDialect) RequiresArrayContainsForInOperator() bool { - return false -} - -func (b *BaseDialect) ArrayContainsAnyExpression(_, _ string) (string, error) { - return "", fmt.Errorf("array contains not supported for %s dialect", b.String()) +func (b *BaseDialect) ArrayContainsAnyExpression(_, _ string) (expr string, ok bool) { + return "", false } func (b *BaseDialect) ArrayContainsSubqueryExpression(_, _, _ string) (expr string, ok bool) { diff --git a/runtime/drivers/duckdb/dialect.go b/runtime/drivers/duckdb/dialect.go index 30eea2de3572..c7fa2ea15977 100644 --- a/runtime/drivers/duckdb/dialect.go +++ b/runtime/drivers/duckdb/dialect.go @@ -38,10 +38,8 @@ func (d *dialect) EscapeTable(db, schema, table string) string { return d.EscapeIdentifier(table) } -func (d *dialect) RequiresArrayContainsForInOperator() bool { return true } - -func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (string, error) { - return fmt.Sprintf("list_has_any(%s, [%s])", arrExpr, valuesExpr), nil +func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr string, ok bool) { + return fmt.Sprintf("list_has_any(%s, [%s])", arrExpr, valuesExpr), true } func (d *dialect) OrderByExpression(name string, desc bool) string { diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index 488b98493743..de806728e726 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -81,17 +81,17 @@ func (b *sqlExprBuilder) writeValue(val any) error { } func (b *sqlExprBuilder) writeSubquery(sub *Subquery) error { - sql, err := b.subquerySQL(sub) + sql, args, err := b.subquerySQL(sub) if err != nil { return err } b.writeString(sql) + b.args = append(b.args, args...) return nil } -// subquerySQL returns "(SELECT FROM ())" and appends the subquery's args to b.args. -// The caller must write the returned SQL before writing any further args. -func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, error) { +// subquerySQL returns "(SELECT FROM ())" and its args. +func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, []any, error) { // We construct a Query that combines the parent Query's contextual info with that of the Subquery. outer := b.ast.Query inner := &Query{ @@ -121,15 +121,14 @@ func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, error) { } innerAST, err := NewAST(b.ast.MetricsView, innerSecurity, inner, b.ast.Dialect) if err != nil { - return "", fmt.Errorf("failed to create AST for subquery: %w", err) + return "", nil, fmt.Errorf("failed to create AST for subquery: %w", err) } sql, args, err := innerAST.SQL() if err != nil { - return "", fmt.Errorf("failed to generate SQL for subquery: %w", err) + return "", nil, fmt.Errorf("failed to generate SQL for subquery: %w", err) } - b.args = append(b.args, args...) // The dimension is selected by its alias, which some dialects (e.g. Snowflake) escape differently from identifiers. - return fmt.Sprintf("(SELECT %s FROM (%s))", b.ast.Dialect.EscapeAlias(sub.Dimension.Name), sql), nil + return fmt.Sprintf("(SELECT %s FROM (%s))", b.ast.Dialect.EscapeAlias(sub.Dimension.Name), sql), args, nil } func (b *sqlExprBuilder) writeCondition(cond *Condition) error { @@ -286,26 +285,40 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) // It avoids scanning the unnested rows and double-counting rows whose array contains multiple matching values. // Subqueries (e.g. from measure filters) only take this path if the dialect can consume them; otherwise they are evaluated against the unnested elements below. if op == OperatorIn || op == OperatorNin { - if vals, ok := right.Value.([]any); ok && b.ast.Dialect.RequiresArrayContainsForInOperator() { - return b.writeArrayContainsCondition(leftExpr, vals, op == OperatorNin) - } - if right.Subquery != nil { - nargs := len(b.args) - sql, err := b.subquerySQL(right.Subquery) - if err != nil { - return err - } - if expr, ok := b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)); ok { - b.writeByte('(') + var expr string + var args []any + var ok bool + if vals, isList := right.Value.([]any); isList { + if len(vals) == 0 { if op == OperatorNin { - b.writeString("NOT ") + b.writeString("TRUE") + } else { + b.writeString("FALSE") } - b.writeString(expr) - b.writeByte(')') return nil } - // The dialect did not use the subquery; drop its args since it is rendered again below. - b.args = b.args[:nargs] + // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. + // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. + expr, ok = b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) + args = vals + } else if right.Subquery != nil { + var sql string + var err error + sql, args, err = b.subquerySQL(right.Subquery) + if err != nil { + return err + } + expr, ok = b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)) + } + if ok { + b.writeByte('(') + if op == OperatorNin { + b.writeString("NOT ") + } + b.writeString(expr) + b.writeByte(')') + b.args = append(b.args, args...) + return nil } } @@ -321,12 +334,10 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) return b.writeBinaryConditionInner(nil, right, leftExpr, op) } // A filter on an unnest dimension that is not selected should match each source row once, even if several of its elements match. - // Prefer the dialect's native any-element expression, then a correlated EXISTS subquery over the unnest join. - // If the dialect can do neither, fall back to joining the unnest into the outer query, which duplicates rows with several matching elements. + // Prefer the dialect's native any-element expression, otherwise use a correlated EXISTS subquery over the unnest join. open, elem, closing, ok := b.ast.Dialect.ArrayAnyExpression(leftExpr, unnestTableAlias) if !ok && !tupleStyle { - b.ast.unnests = append(b.ast.unnests, unnestFrom) - return b.writeBinaryConditionInner(nil, right, b.ast.Dialect.EscapeAlias(left.Name), op) + return fmt.Errorf("dialect %s cannot filter on unnest dimension %q: it must support tuple-style unnest or an array any-element expression", b.ast.Dialect, left.Name) } if !ok { open = "EXISTS (SELECT 1 FROM " + unnestFrom + " WHERE " @@ -684,33 +695,6 @@ func (b *sqlExprBuilder) writeInConditionForValues(left *Expression, leftOverrid return nil } -func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, vals []any, not bool) error { - if len(vals) == 0 { - if not { - b.writeString("TRUE") - } else { - b.writeString("FALSE") - } - return nil - } - - b.writeByte('(') - if not { - b.writeString("NOT ") - } - // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. - // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. - b.args = append(b.args, vals...) - expr, err := b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) - if err != nil { - return err - } - b.writeString(expr) - b.writeByte(')') - - return nil -} - func (b *sqlExprBuilder) writeByte(v byte) { _ = b.out.WriteByte(v) } From e39915d5102e293d1f1dc0c1345950c7a5900f56 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Tue, 15 Sep 2026 16:21:57 +0530 Subject: [PATCH 7/8] mroe simplifications --- runtime/metricsview/astexpr.go | 79 +++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 29 deletions(-) diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index de806728e726..0f78d0a83bd4 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -127,8 +127,14 @@ func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, []any, error) { if err != nil { return "", nil, fmt.Errorf("failed to generate SQL for subquery: %w", err) } - // The dimension is selected by its alias, which some dialects (e.g. Snowflake) escape differently from identifiers. - return fmt.Sprintf("(SELECT %s FROM (%s))", b.ast.Dialect.EscapeAlias(sub.Dimension.Name), sql), args, nil + // Output: (SELECT FROM ()) + var out strings.Builder + out.WriteString("(SELECT ") + out.WriteString(b.ast.Dialect.EscapeAlias(sub.Dimension.Name)) + out.WriteString(" FROM (") + out.WriteString(sql) + out.WriteString("))") + return out.String(), args, nil } func (b *sqlExprBuilder) writeCondition(cond *Condition) error { @@ -285,40 +291,25 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) // It avoids scanning the unnested rows and double-counting rows whose array contains multiple matching values. // Subqueries (e.g. from measure filters) only take this path if the dialect can consume them; otherwise they are evaluated against the unnested elements below. if op == OperatorIn || op == OperatorNin { - var expr string - var args []any - var ok bool - if vals, isList := right.Value.([]any); isList { - if len(vals) == 0 { - if op == OperatorNin { - b.writeString("TRUE") - } else { - b.writeString("FALSE") - } + if vals, ok := right.Value.([]any); ok { + if b.writeArrayContainsCondition(leftExpr, vals, op == OperatorNin) { return nil } - // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. - // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. - expr, ok = b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) - args = vals } else if right.Subquery != nil { - var sql string - var err error - sql, args, err = b.subquerySQL(right.Subquery) + sql, args, err := b.subquerySQL(right.Subquery) if err != nil { return err } - expr, ok = b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)) - } - if ok { - b.writeByte('(') - if op == OperatorNin { - b.writeString("NOT ") + if expr, ok := b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)); ok { + b.writeByte('(') + if op == OperatorNin { + b.writeString("NOT ") + } + b.writeString(expr) + b.writeByte(')') + b.args = append(b.args, args...) + return nil } - b.writeString(expr) - b.writeByte(')') - b.args = append(b.args, args...) - return nil } } @@ -695,6 +686,36 @@ func (b *sqlExprBuilder) writeInConditionForValues(left *Expression, leftOverrid return nil } +// writeArrayContainsCondition writes a native array-contains condition for vals. +// It returns false without writing anything if the dialect has no such expression. +func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, vals []any, not bool) bool { + if len(vals) == 0 { + if not { + b.writeString("TRUE") + } else { + b.writeString("FALSE") + } + return true + } + + // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. + // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. + expr, ok := b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) + if !ok { + return false + } + + b.writeByte('(') + if not { + b.writeString("NOT ") + } + b.writeString(expr) + b.writeByte(')') + b.args = append(b.args, vals...) + + return true +} + func (b *sqlExprBuilder) writeByte(v byte) { _ = b.out.WriteByte(v) } From 5ba8c4d0af5b6d0a001d761245fa0fd3ce0ab704 Mon Sep 17 00:00:00 2001 From: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Date: Wed, 16 Sep 2026 13:40:34 +0530 Subject: [PATCH 8/8] handle subquery later --- runtime/drivers/bigquery/dialect.go | 7 -- runtime/drivers/bigquery/olap_test.go | 16 ----- runtime/drivers/databricks/dialect.go | 5 -- runtime/drivers/databricks/olap_test.go | 16 ----- runtime/drivers/dialect.go | 6 -- runtime/drivers/snowflake/dialect.go | 6 -- runtime/drivers/snowflake/olap_test.go | 16 ----- runtime/metricsview/ast_unnest_test.go | 85 ++++++++++++++----------- runtime/metricsview/astexpr.go | 71 ++++++++------------- 9 files changed, 73 insertions(+), 155 deletions(-) diff --git a/runtime/drivers/bigquery/dialect.go b/runtime/drivers/bigquery/dialect.go index d7e51506421a..18171fde60fc 100644 --- a/runtime/drivers/bigquery/dialect.go +++ b/runtime/drivers/bigquery/dialect.go @@ -88,13 +88,6 @@ func (d *dialect) LateralUnnest(expr, _, colName string) (tbl string, tupleStyle return fmt.Sprintf(`UNNEST(%s) AS %s`, expr, d.EscapeIdentifier(colName)), false, false, nil } -// ArrayContainsSubqueryExpression joins the subquery to the unnested array. -// BigQuery cannot de-correlate an IN subquery that references another table inside a correlated EXISTS. -// UNNEST comes first so that the array expression resolves against the outer query and cannot be shadowed by the subquery's column. -func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { - return fmt.Sprintf("EXISTS (SELECT 1 FROM UNNEST(%s) AS e JOIN %s AS s ON e = s.%s)", arrExpr, subquerySQL, valueCol), true -} - func (d *dialect) ArrayAnyExpression(arrExpr, elemAlias string) (open, elem, closing string, ok bool) { elem = d.EscapeIdentifier(elemAlias) return fmt.Sprintf("EXISTS (SELECT 1 FROM UNNEST(%s) AS %s WHERE ", arrExpr, elem), elem, ")", true diff --git a/runtime/drivers/bigquery/olap_test.go b/runtime/drivers/bigquery/olap_test.go index cfccc50604dc..c4350e721386 100644 --- a/runtime/drivers/bigquery/olap_test.go +++ b/runtime/drivers/bigquery/olap_test.go @@ -214,22 +214,6 @@ func TestUnnestDimension(t *testing.T) { qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": int64(0)}}, }, - { - // Measure filter: the only dimension value with more than one row is 'b'. - name: "in filter with measure-filter subquery", - qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ - Operator: metricsview.OperatorIn, - Expressions: []*metricsview.Expression{ - {Name: "tags"}, - {Subquery: &metricsview.Subquery{ - Dimension: metricsview.Dimension{Name: "tags"}, - Measures: []metricsview.Measure{{Name: "count"}}, - Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, - }}, - }, - }}), - want: []map[string]any{{"count": int64(2)}}, - }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/drivers/databricks/dialect.go b/runtime/drivers/databricks/dialect.go index c099bfafcc38..c93965ef2072 100644 --- a/runtime/drivers/databricks/dialect.go +++ b/runtime/drivers/databricks/dialect.go @@ -91,11 +91,6 @@ func (d *dialect) ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr s return fmt.Sprintf("COALESCE(arrays_overlap(%s, array(%s)), FALSE)", arrExpr, valuesExpr), true } -// ArrayContainsSubqueryExpression collects the subquery into an array because Databricks does not allow subqueries inside lambda functions. -func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { - return fmt.Sprintf("COALESCE(arrays_overlap(%s, (SELECT collect_list(s.%s) FROM %s AS s)), FALSE)", arrExpr, valueCol, subquerySQL), true -} - func (d *dialect) DateTruncExpr(dim *runtimev1.MetricsViewSpec_Dimension, grain runtimev1.TimeGrain, tz string, firstDayOfWeek, firstMonthOfYear int) (string, error) { if tz == "UTC" || tz == "Etc/UTC" { tz = "" diff --git a/runtime/drivers/databricks/olap_test.go b/runtime/drivers/databricks/olap_test.go index a9ac239dd901..116978fc16ab 100644 --- a/runtime/drivers/databricks/olap_test.go +++ b/runtime/drivers/databricks/olap_test.go @@ -271,22 +271,6 @@ func TestUnnestDimension(t *testing.T) { qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": int64(0)}}, }, - { - // Measure filter: dimension values with more than one row are 'b' and 'c'. - name: "in filter with measure-filter subquery", - qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ - Operator: metricsview.OperatorIn, - Expressions: []*metricsview.Expression{ - {Name: "tags"}, - {Subquery: &metricsview.Subquery{ - Dimension: metricsview.Dimension{Name: "tags"}, - Measures: []metricsview.Measure{{Name: "count"}}, - Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, - }}, - }, - }}), - want: []map[string]any{{"count": int64(4)}}, - }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index 8588f97807ea..559900d1d0c9 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -50,8 +50,6 @@ type Dialect interface { // ArrayContainsAnyExpression returns an expression that is true if the array arrExpr contains any of the comma-separated valuesExpr. // ok is false if the dialect has no such expression, in which case the condition is evaluated against the unnested elements instead. ArrayContainsAnyExpression(arrExpr, valuesExpr string) (expr string, ok bool) - // ArrayContainsSubqueryExpression is like ArrayContainsAnyExpression but takes a parenthesized subquery whose values are in the column valueCol. - ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) // LateralUnnest returns the join clause that unnests expr. If tupleStyle is false the element is referenced by colName alone, // and the dialect must implement ArrayAnyExpression since it cannot be referenced from a correlated subquery. @@ -249,10 +247,6 @@ func (b *BaseDialect) ArrayContainsAnyExpression(_, _ string) (expr string, ok b return "", false } -func (b *BaseDialect) ArrayContainsSubqueryExpression(_, _, _ string) (expr string, ok bool) { - return "", false -} - func (b *BaseDialect) MetricsViewDimensionExpression(dimension *runtimev1.MetricsViewSpec_Dimension) (string, error) { if dimension.LookupTable != "" { return "", fmt.Errorf("lookup tables are not supported for %s dialect", b.String()) diff --git a/runtime/drivers/snowflake/dialect.go b/runtime/drivers/snowflake/dialect.go index 6df37fb0c3dc..486ba532aecb 100644 --- a/runtime/drivers/snowflake/dialect.go +++ b/runtime/drivers/snowflake/dialect.go @@ -72,12 +72,6 @@ func (d *dialect) UnnestedColumn(tableAlias, colName string) string { return d.EscapeMember(tableAlias, colName) + "::VARCHAR" } -// ArrayContainsSubqueryExpression aggregates the subquery into an array and tests each element against it. -// An IN subquery inside the FILTER lambda causes an internal error in Snowflake, and ARRAYS_OVERLAP does not accept structured arrays. -func (d *dialect) ArrayContainsSubqueryExpression(arrExpr, subquerySQL, valueCol string) (expr string, ok bool) { - return fmt.Sprintf("COALESCE(ARRAY_SIZE(FILTER(%s, x -> ARRAY_CONTAINS(x::VARCHAR::VARIANT, (SELECT ARRAY_AGG(s.%s) FROM %s AS s)))) > 0, FALSE)", arrExpr, valueCol, subquerySQL), true -} - // ArrayAnyExpression uses FILTER because Snowflake rejects correlated FLATTEN inside EXISTS subqueries. // It also serves IN filters: ARRAYS_OVERLAP does not accept structured arrays and compares raw VARIANT elements, which would not match the VARCHAR values shown by UnnestedColumn. // FILTER(NULL, ...) is NULL, so the result is coalesced to FALSE to keep rows with a NULL array under negated filters. diff --git a/runtime/drivers/snowflake/olap_test.go b/runtime/drivers/snowflake/olap_test.go index c275a5e95da4..e10be5a9bee9 100644 --- a/runtime/drivers/snowflake/olap_test.go +++ b/runtime/drivers/snowflake/olap_test.go @@ -286,22 +286,6 @@ func TestUnnestDimension(t *testing.T) { qry: count(tagsFilter(metricsview.OperatorEq, "missing")), want: []map[string]any{{"count": "0"}}, }, - { - // Measure filter: dimension values with more than one row are 'b' and 'c'. - name: "in filter with measure-filter subquery", - qry: count(&metricsview.Expression{Condition: &metricsview.Condition{ - Operator: metricsview.OperatorIn, - Expressions: []*metricsview.Expression{ - {Name: "tags"}, - {Subquery: &metricsview.Subquery{ - Dimension: metricsview.Dimension{Name: "tags"}, - Measures: []metricsview.Measure{{Name: "count"}}, - Having: &metricsview.Expression{Condition: &metricsview.Condition{Operator: metricsview.OperatorGt, Expressions: []*metricsview.Expression{{Name: "count"}, {Value: 1}}}}, - }}, - }, - }}), - want: []map[string]any{{"count": "4"}}, - }, { name: "filter combined with group by on another dimension", qry: &metricsview.Query{ diff --git a/runtime/metricsview/ast_unnest_test.go b/runtime/metricsview/ast_unnest_test.go index c2ff1ecaf568..1bb1d95686ce 100644 --- a/runtime/metricsview/ast_unnest_test.go +++ b/runtime/metricsview/ast_unnest_test.go @@ -6,7 +6,9 @@ import ( runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/drivers/bigquery" + "github.com/rilldata/rill/runtime/drivers/clickhouse" "github.com/rilldata/rill/runtime/drivers/databricks" + "github.com/rilldata/rill/runtime/drivers/druid" "github.com/rilldata/rill/runtime/drivers/duckdb" "github.com/rilldata/rill/runtime/drivers/snowflake" "github.com/stretchr/testify/require" @@ -113,7 +115,7 @@ func TestUnnestSQL(t *testing.T) { } } -// Measure filters produce "dim IN (subquery)". Dialects with an array-contains fast path must still handle them. +// Subquery filters remain supported for scalar dimensions and existing general unnest paths. func TestUnnestSubqueryFilterSQL(t *testing.T) { mv := &runtimev1.MetricsViewSpec{ Table: "test_table", @@ -125,46 +127,55 @@ func TestUnnestSubqueryFilterSQL(t *testing.T) { {Name: "count", Expression: "count(*)", Type: runtimev1.MetricsViewSpec_MEASURE_TYPE_SIMPLE}, }, } - where := &Expression{Condition: &Condition{ - Operator: OperatorNin, - Expressions: []*Expression{ - {Name: "tags"}, - {Subquery: &Subquery{ - Dimension: Dimension{Name: "tags"}, - Measures: []Measure{{Name: "count"}}, - Having: &Expression{Condition: &Condition{Operator: OperatorGt, Expressions: []*Expression{{Name: "count"}, {Value: 10}}}}, - }}, - }, - }} - // The subquery is the metrics view grouped by the unnest dimension, with the having clause applied in an outer select. - sub := map[string]string{ - "duckdb": `(SELECT "tags" FROM (SELECT ("t2"."tags") AS "tags", ("t2"."count") AS "count" FROM (SELECT ("t0"."tags") AS "tags", (count(*)) AS "count" FROM "test_table", LATERAL UNNEST("tags") t0("tags") GROUP BY 1) t2 WHERE (("t2"."count") > ?)))`, - "databricks": "(SELECT `tags` FROM (SELECT (`t2`.`tags`) AS `tags`, (`t2`.`count`) AS `count` FROM (SELECT (`t0`.`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table` LATERAL VIEW EXPLODE(`tags`) t0 AS `tags` GROUP BY 1) t2 WHERE ((`t2`.`count`) > ?)))", - "snowflake": `(SELECT "tags" FROM (SELECT (t2."tags") AS "tags", (t2."count") AS "count" FROM (SELECT (t0.tags::VARCHAR) AS "tags", (count(*)) AS "count" FROM test_table, LATERAL FLATTEN(INPUT => tags) t0 (seq, key, path, index, tags, this) GROUP BY 1) t2 WHERE ((t2."count") > ?)))`, - "bigquery": "(SELECT `tags` FROM (SELECT (`t2`.`tags`) AS `tags`, (`t2`.`count`) AS `count` FROM (SELECT (`tags`) AS `tags`, (count(*)) AS `count` FROM `test_table`, UNNEST(`tags`) AS `tags` GROUP BY 1) t2 WHERE ((`t2`.`count`) > ?)))", - } + base := drivers.NewBaseDialect(drivers.DialectNamePostgres, drivers.DoubleQuotesEscapeIdentifier, drivers.DoubleQuotesEscapeIdentifier) tests := []struct { dialect drivers.Dialect - want string + wantErr string }{ - // No native form: correlated EXISTS over the unnest join. - {duckdb.DialectDuckDB, `WHERE NOT EXISTS (SELECT 1 FROM LATERAL UNNEST("tags") t0("tags") WHERE (("t0"."tags") IN ` + sub["duckdb"] + `)) GROUP BY 1`}, - // Lambdas cannot contain subqueries: aggregate the subquery into an array. - {databricks.DialectDatabricks, "WHERE (NOT COALESCE(arrays_overlap((`tags`), (SELECT collect_list(s.`tags`) FROM " + sub["databricks"] + " AS s)), FALSE)) GROUP BY 1"}, - // IN subquery inside FILTER hits an internal error: aggregate into an array and use ARRAY_CONTAINS. - {snowflake.DialectSnowflake, `WHERE (NOT COALESCE(ARRAY_SIZE(FILTER((tags), x -> ARRAY_CONTAINS(x::VARCHAR::VARIANT, (SELECT ARRAY_AGG(s."tags") FROM ` + sub["snowflake"] + ` AS s)))) > 0, FALSE)) GROUP BY 1`}, - // A table-referencing subquery inside correlated EXISTS cannot be de-correlated: join the subquery to the unnested array instead. - {bigquery.DialectBigQuery, "WHERE (NOT EXISTS (SELECT 1 FROM UNNEST((`tags`)) AS e JOIN " + sub["bigquery"] + " AS s ON e = s.`tags`)) GROUP BY 1"}, + {duckdb.DialectDuckDB, "the right value must be a list of values for an array IN condition"}, + {clickhouse.DialectClickhouse, "the right value must be a list of values for an array IN condition"}, + {databricks.DialectDatabricks, "the right value must be a list of values for an array IN condition"}, + {snowflake.DialectSnowflake, `dialect snowflake does not support subquery filters on unnest dimension "tags"`}, + {bigquery.DialectBigQuery, `dialect bigquery does not support subquery filters on unnest dimension "tags"`}, + {druid.DialectDruid, ""}, + {&base, ""}, } for _, tt := range tests { - t.Run(tt.dialect.String(), func(t *testing.T) { - qry := &Query{MetricsView: "test", Dimensions: []Dimension{{Name: "city"}}, Measures: []Measure{{Name: "count"}}, Where: where} - ast, err := NewAST(mv, skipMetricsViewSecurity{}, qry, tt.dialect) - require.NoError(t, err) - sql, args, err := ast.SQL() - require.NoError(t, err) - require.Contains(t, sql, tt.want) - require.Equal(t, []any{10}, args) - }) + for _, op := range []Operator{OperatorIn, OperatorNin} { + for _, shape := range []struct { + name string + dim string + dims []Dimension + }{ + {"unselected unnest dimension", "tags", []Dimension{{Name: "city"}}}, + {"selected unnest dimension", "tags", []Dimension{{Name: "tags"}}}, + {"scalar dimension", "city", []Dimension{{Name: "city"}}}, + } { + t.Run(tt.dialect.String()+"/"+string(op)+"/"+shape.name, func(t *testing.T) { + where := &Expression{Condition: &Condition{ + Operator: op, + Expressions: []*Expression{ + {Name: shape.dim}, + {Subquery: &Subquery{ + Dimension: Dimension{Name: shape.dim}, + Measures: []Measure{{Name: "count"}}, + Having: &Expression{Condition: &Condition{Operator: OperatorGt, Expressions: []*Expression{{Name: "count"}, {Value: 10}}}}, + }}, + }, + }} + qry := &Query{MetricsView: "test", Dimensions: shape.dims, Measures: []Measure{{Name: "count"}}, Where: where} + ast, err := NewAST(mv, skipMetricsViewSecurity{}, qry, tt.dialect) + if shape.name == "unselected unnest dimension" && tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + return + } + require.NoError(t, err) + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Contains(t, sql, " IN (SELECT "+tt.dialect.EscapeAlias(shape.dim)+" FROM (") + require.Equal(t, []any{10}, args) + }) + } + } } } diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index 0f78d0a83bd4..a626274f829f 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -81,17 +81,6 @@ func (b *sqlExprBuilder) writeValue(val any) error { } func (b *sqlExprBuilder) writeSubquery(sub *Subquery) error { - sql, args, err := b.subquerySQL(sub) - if err != nil { - return err - } - b.writeString(sql) - b.args = append(b.args, args...) - return nil -} - -// subquerySQL returns "(SELECT FROM ())" and its args. -func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, []any, error) { // We construct a Query that combines the parent Query's contextual info with that of the Subquery. outer := b.ast.Query inner := &Query{ @@ -121,20 +110,21 @@ func (b *sqlExprBuilder) subquerySQL(sub *Subquery) (string, []any, error) { } innerAST, err := NewAST(b.ast.MetricsView, innerSecurity, inner, b.ast.Dialect) if err != nil { - return "", nil, fmt.Errorf("failed to create AST for subquery: %w", err) + return fmt.Errorf("failed to create AST for subquery: %w", err) } sql, args, err := innerAST.SQL() if err != nil { - return "", nil, fmt.Errorf("failed to generate SQL for subquery: %w", err) + return fmt.Errorf("failed to generate SQL for subquery: %w", err) } + // Output: (SELECT FROM ()) - var out strings.Builder - out.WriteString("(SELECT ") - out.WriteString(b.ast.Dialect.EscapeAlias(sub.Dimension.Name)) - out.WriteString(" FROM (") - out.WriteString(sql) - out.WriteString("))") - return out.String(), args, nil + b.writeString("(SELECT ") + b.writeString(b.ast.Dialect.EscapeAlias(sub.Dimension.Name)) + b.writeString(" FROM (") + b.writeString(sql) + b.writeString("))") + b.args = append(b.args, args...) + return nil } func (b *sqlExprBuilder) writeCondition(cond *Condition) error { @@ -289,27 +279,9 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) // For IN/NIN on unnest dimensions, prefer a native array-contains expression over an unnest join where the dialect supports it. // It avoids scanning the unnested rows and double-counting rows whose array contains multiple matching values. - // Subqueries (e.g. from measure filters) only take this path if the dialect can consume them; otherwise they are evaluated against the unnested elements below. if op == OperatorIn || op == OperatorNin { - if vals, ok := right.Value.([]any); ok { - if b.writeArrayContainsCondition(leftExpr, vals, op == OperatorNin) { - return nil - } - } else if right.Subquery != nil { - sql, args, err := b.subquerySQL(right.Subquery) - if err != nil { - return err - } - if expr, ok := b.ast.Dialect.ArrayContainsSubqueryExpression("("+leftExpr+")", sql, b.ast.Dialect.EscapeAlias(right.Subquery.Dimension.Name)); ok { - b.writeByte('(') - if op == OperatorNin { - b.writeString("NOT ") - } - b.writeString(expr) - b.writeByte(')') - b.args = append(b.args, args...) - return nil - } + if handled, err := b.writeArrayContainsCondition(leftExpr, right, op == OperatorNin); handled || err != nil { + return err } } @@ -327,6 +299,9 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) // A filter on an unnest dimension that is not selected should match each source row once, even if several of its elements match. // Prefer the dialect's native any-element expression, otherwise use a correlated EXISTS subquery over the unnest join. open, elem, closing, ok := b.ast.Dialect.ArrayAnyExpression(leftExpr, unnestTableAlias) + if ok && right.Subquery != nil { + return fmt.Errorf("dialect %s does not support subquery filters on unnest dimension %q", b.ast.Dialect, left.Name) + } if !ok && !tupleStyle { return fmt.Errorf("dialect %s cannot filter on unnest dimension %q: it must support tuple-style unnest or an array any-element expression", b.ast.Dialect, left.Name) } @@ -686,23 +661,27 @@ func (b *sqlExprBuilder) writeInConditionForValues(left *Expression, leftOverrid return nil } -// writeArrayContainsCondition writes a native array-contains condition for vals. +// writeArrayContainsCondition writes a native array-contains condition for a list of values. // It returns false without writing anything if the dialect has no such expression. -func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, vals []any, not bool) bool { - if len(vals) == 0 { +func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, right *Expression, not bool) (bool, error) { + vals, isList := right.Value.([]any) + if isList && len(vals) == 0 { if not { b.writeString("TRUE") } else { b.writeString("FALSE") } - return true + return true, nil } // NULL values in the list are not handled separately: ClickHouse's hasAny matches them, while DuckDB's list_has_any and Databricks' arrays_overlap ignore them. // There is no reliable way to check for NULL elements; leftExpr IS NULL checks for a NULL array, not NULL elements. expr, ok := b.ast.Dialect.ArrayContainsAnyExpression("("+leftExpr+")", strings.TrimSuffix(strings.Repeat("?,", len(vals)), ",")) if !ok { - return false + return false, nil + } + if !isList { + return false, fmt.Errorf("the right value must be a list of values for an array IN condition") } b.writeByte('(') @@ -713,7 +692,7 @@ func (b *sqlExprBuilder) writeArrayContainsCondition(leftExpr string, vals []any b.writeByte(')') b.args = append(b.args, vals...) - return true + return true, nil } func (b *sqlExprBuilder) writeByte(v byte) {