diff --git a/runtime/drivers/dialect.go b/runtime/drivers/dialect.go index 445f2ee5323a..b46e72f083ee 100644 --- a/runtime/drivers/dialect.go +++ b/runtime/drivers/dialect.go @@ -47,6 +47,8 @@ type Dialect interface { GetCastExprForLike() string SupportsRegexMatch() bool GetRegexMatchFunction() (string, error) + // GetRegexMatchCastExpr returns expr cast to the string type accepted by the dialect's regex match function. + GetRegexMatchCastExpr(expr string) (string, error) RequiresArrayContainsForInOperator() bool GetArrayContainsFunction() (string, error) DimensionSelect(escapeTable string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) @@ -162,6 +164,10 @@ func (b *BaseDialect) GetRegexMatchFunction() (string, error) { return "", fmt.Errorf("regex match not supported for %s dialect", b.String()) } +func (b *BaseDialect) GetRegexMatchCastExpr(expr string) (string, error) { + return "", fmt.Errorf("regex match not supported for %s dialect", b.String()) +} + // EscapeTable returns an escaped table name with database, schema and table. func (b *BaseDialect) EscapeTable(db, schema, table string) string { var sb strings.Builder diff --git a/runtime/drivers/druid/dialect.go b/runtime/drivers/druid/dialect.go index 68ba2a2a6f60..c1263e0ceb96 100644 --- a/runtime/drivers/druid/dialect.go +++ b/runtime/drivers/druid/dialect.go @@ -27,6 +27,11 @@ func (d *dialect) SupportsRegexMatch() bool { return true } func (d *dialect) GetRegexMatchFunction() (string, error) { return "REGEXP_LIKE", nil } +// GetRegexMatchCastExpr casts expr to VARCHAR since REGEXP_LIKE only accepts string operands (it rejects TIMESTAMP and numeric columns). +func (d *dialect) GetRegexMatchCastExpr(expr string) (string, error) { + return fmt.Sprintf("CAST(%s AS VARCHAR)", expr), nil +} + // DimensionSelect for Druid skips unnesting even when dim.Unnest is true. func (d *dialect) DimensionSelect(_ string, dim *runtimev1.MetricsViewSpec_Dimension) (dimSelect, unnestClause string, err error) { alias := d.EscapeAlias(dim.Name) diff --git a/runtime/metricsview/astexpr.go b/runtime/metricsview/astexpr.go index a3eb41e39dd9..271008e459d8 100644 --- a/runtime/metricsview/astexpr.go +++ b/runtime/metricsview/astexpr.go @@ -274,7 +274,8 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) return nil } - return b.writeBinaryConditionInner(nil, right, leftExpr, op) + // The left expression is passed along with its rendered SQL so the inner writers can look up the dimension's data type. + return b.writeBinaryConditionInner(left, right, leftExpr, op) } // For IN/NIN on unnest dimensions backed by DuckDB or ClickHouse, use native array-contains @@ -292,7 +293,7 @@ func (b *sqlExprBuilder) writeBinaryCondition(exprs []*Expression, op Operator) if auto { // Means the DB automatically unnests, so we can treat it as a normal value leftExpr = b.ast.Dialect.AutoUnnest(leftExpr) - return b.writeBinaryConditionInner(nil, right, leftExpr, op) + return b.writeBinaryConditionInner(left, right, leftExpr, op) } var unnestColAlias string if tupleStyle { @@ -429,6 +430,7 @@ func (b *sqlExprBuilder) writeILikeCondition(left, right *Expression, leftOverri b.writeString(b.ast.Dialect.GetCastExprForLike()) } else if b.ast.Dialect.SupportsRegexMatch() { + // Output: [NOT] (, ) [OR IS NULL] if not { b.writeString(" NOT ") } @@ -438,7 +440,15 @@ func (b *sqlExprBuilder) writeILikeCondition(left, right *Expression, leftOverri } b.writeString(regexFunc) b.writeByte('(') - if leftOverride != "" { + if leftOverride != "" && b.needsStringCastForRegexMatch(left) { + // Regex match functions only accept string operands, so a known non-string dimension is cast. + // A dimension reference always arrives with its rendered SQL in leftOverride (see writeBinaryCondition). + expr, err := b.ast.Dialect.GetRegexMatchCastExpr("(" + leftOverride + ")") + if err != nil { + return err + } + b.writeString(expr) + } else if leftOverride != "" { b.writeParenthesizedString(leftOverride) } else { err := b.writeExpression(left) @@ -521,6 +531,20 @@ func (b *sqlExprBuilder) writeILikeCondition(left, right *Expression, leftOverri return nil } +// needsStringCastForRegexMatch reports whether the left operand of a regex match must be cast to a string. +// A dimension is cast if its resolved data type is known and not a string. +// Dimensions of unknown type and expressions that are not a plain dimension reference are never cast. +func (b *sqlExprBuilder) needsStringCastForRegexMatch(left *Expression) bool { + if left == nil || left.Name == "" { + return false + } + dim, err := b.ast.LookupDimension(left.Name, b.visible) + if err != nil || dim.DataType == nil { + return false + } + return dim.DataType.Code != runtimev1.Type_CODE_UNSPECIFIED && dim.DataType.Code != runtimev1.Type_CODE_STRING +} + func (b *sqlExprBuilder) writeInCondition(left, right *Expression, leftOverride string, not bool) error { if right.Value != nil { vals, ok := right.Value.([]any) diff --git a/runtime/metricsview/executor/executor.go b/runtime/metricsview/executor/executor.go index 701a94a6c066..0a77bad6f78a 100644 --- a/runtime/metricsview/executor/executor.go +++ b/runtime/metricsview/executor/executor.go @@ -644,7 +644,8 @@ func (e *Executor) Search(ctx context.Context, qry *metricsview.SearchQuery, exe if err != nil { return nil, err } - finalSQL.WriteString(fmt.Sprintf("SELECT %s AS dimension, %s AS value FROM (%s)", drivers.EscapeStringValue(d), e.olap.Dialect().EscapeIdentifier(d), sql)) + // The aliases must be escaped: "value" is a reserved keyword in Druid (Calcite) SQL. + finalSQL.WriteString(fmt.Sprintf("SELECT %s AS %s, %s AS %s FROM (%s)", drivers.EscapeStringValue(d), e.olap.Dialect().EscapeAlias("dimension"), e.olap.Dialect().EscapeIdentifier(d), e.olap.Dialect().EscapeAlias("value"), sql)) finalArgs = append(finalArgs, args...) } diff --git a/runtime/metricsview/executor/executor_search_druid_test.go b/runtime/metricsview/executor/executor_search_druid_test.go new file mode 100644 index 000000000000..10bcee6bf7cd --- /dev/null +++ b/runtime/metricsview/executor/executor_search_druid_test.go @@ -0,0 +1,58 @@ +package executor + +import ( + "context" + "errors" + "testing" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/rilldata/rill/runtime" + "github.com/rilldata/rill/runtime/drivers" + "github.com/rilldata/rill/runtime/drivers/druid" + "github.com/rilldata/rill/runtime/metricsview" + "github.com/stretchr/testify/require" +) + +// errSQLCaptured is returned by sqlCaptureOLAP.Query so that a test can stop the executor right after the SQL is generated. +var errSQLCaptured = errors.New("sql captured") + +// sqlCaptureOLAP is a stub OLAP store that reports the Druid dialect and records the statement passed to Query instead of executing it. +type sqlCaptureOLAP struct { + drivers.OLAPStore + stmt *drivers.Statement +} + +func (o *sqlCaptureOLAP) Dialect() drivers.Dialect { return druid.DialectDruid } + +func (o *sqlCaptureOLAP) Query(_ context.Context, stmt *drivers.Statement) (*drivers.Result, error) { + o.stmt = stmt + return nil, errSQLCaptured +} + +// TestSearchDruidFallbackSQL checks the full UNION ALL query that Search issues to Druid when native search is unavailable (here because the query has no time range). +// The outer aliases must be quoted since "value" is a reserved keyword in Druid (Calcite) SQL. +// The per-dimension inner queries are covered separately by TestSearchDruidSQLCastsNonStringDimensions. +func TestSearchDruidFallbackSQL(t *testing.T) { + olap := &sqlCaptureOLAP{} + e := &Executor{ + metricsView: &runtimev1.MetricsViewSpec{ + Table: "events", + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "publisher", Column: "publisher"}, + {Name: "domain", Column: "domain"}, + }, + }, + security: runtime.ResolvedSecurityOpen, + olap: olap, + } + + _, err := e.Search(context.Background(), &metricsview.SearchQuery{ + MetricsView: "mv", + Dimensions: []string{"publisher", "domain"}, + Search: "oo", + }, nil) + require.ErrorIs(t, err, errSQLCaptured) + require.NotNil(t, olap.stmt) + require.Equal(t, `SELECT 'publisher' AS "dimension", "publisher" AS "value" FROM (SELECT ("publisher") AS "publisher" FROM "events" WHERE (REGEXP_LIKE(("publisher"), ?)) GROUP BY 1) UNION ALL SELECT 'domain' AS "dimension", "domain" AS "value" FROM (SELECT ("domain") AS "domain" FROM "events" WHERE (REGEXP_LIKE(("domain"), ?)) GROUP BY 1)`, olap.stmt.Query) + require.Equal(t, []any{"^(?i).*oo.*$", "^(?i).*oo.*$"}, olap.stmt.Args) +} diff --git a/runtime/metricsview/executor/executor_search_test.go b/runtime/metricsview/executor/executor_search_test.go new file mode 100644 index 000000000000..baec2d58dd4c --- /dev/null +++ b/runtime/metricsview/executor/executor_search_test.go @@ -0,0 +1,134 @@ +package executor_test + +import ( + "context" + "testing" + "time" + + runtimev1 "github.com/rilldata/rill/proto/gen/rill/runtime/v1" + "github.com/rilldata/rill/runtime" + "github.com/rilldata/rill/runtime/drivers/druid" + "github.com/rilldata/rill/runtime/metricsview" + "github.com/rilldata/rill/runtime/metricsview/executor" + "github.com/rilldata/rill/runtime/testruntime" + "github.com/stretchr/testify/require" + + _ "github.com/rilldata/rill/runtime/resolvers" +) + +func TestSearch(t *testing.T) { + rt, instanceID := testruntime.NewInstanceWithOptions(t, testruntime.InstanceOptions{ + Files: map[string]string{ + "rill.yaml": "", + "models/events.sql": ` +SELECT * FROM (VALUES + (TIMESTAMP '2024-01-01 00:00:00', 'Google', 'news.com'), + (TIMESTAMP '2024-01-02 00:00:00', 'Facebook', 'sports.com'), + (TIMESTAMP '2024-01-03 00:00:00', 'Microsoft', 'foo.com'), + (TIMESTAMP '2024-02-01 00:00:00', 'Yahoo', 'news.com') +) t(timestamp, publisher, domain) +`, + "metrics_views/mv.yaml": ` +type: metrics_view +version: 1 +model: events +timeseries: timestamp +dimensions: + - name: publisher + column: publisher + - name: domain + column: domain +measures: + - name: count + expression: count(*) +`, + }, + }) + testruntime.RequireReconcileState(t, rt, instanceID, 3, 0, 0) + + r := testruntime.GetResource(t, rt, instanceID, runtime.ResourceKindMetricsView, "mv") + mv := r.GetMetricsView().State.ValidSpec + require.NotNil(t, mv) + + e, err := executor.New(context.Background(), rt, instanceID, mv, false, runtime.ResolvedSecurityOpen, 0, nil) + require.NoError(t, err) + defer e.Close() + + t.Run("multiple dimensions", func(t *testing.T) { + res, err := e.Search(context.Background(), &metricsview.SearchQuery{ + MetricsView: "mv", + Dimensions: []string{"publisher", "domain"}, + Search: "oo", + }, nil) + require.NoError(t, err) + require.ElementsMatch(t, []metricsview.SearchResult{ + {Dimension: "publisher", Value: "Google"}, + {Dimension: "publisher", Value: "Facebook"}, + {Dimension: "publisher", Value: "Yahoo"}, + {Dimension: "domain", Value: "foo.com"}, + }, res) + }) + + t.Run("with where and time range", func(t *testing.T) { + res, err := e.Search(context.Background(), &metricsview.SearchQuery{ + MetricsView: "mv", + Dimensions: []string{"publisher", "domain"}, + Search: "oo", + Where: &metricsview.Expression{Condition: &metricsview.Condition{ + Operator: metricsview.OperatorNeq, + Expressions: []*metricsview.Expression{{Name: "domain"}, {Value: "sports.com"}}, + }}, + TimeRange: &metricsview.TimeRange{ + Start: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), + End: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), + }, + }, nil) + require.NoError(t, err) + require.ElementsMatch(t, []metricsview.SearchResult{ + {Dimension: "publisher", Value: "Google"}, + {Dimension: "domain", Value: "foo.com"}, + }, res) + }) +} + +// TestSearchDruidSQLCastsNonStringDimensions checks the per-dimension SQL that the search fallback (UNION ALL) query generates for Druid. +// Druid has no ILIKE, so the search is compiled to REGEXP_LIKE, which only accepts string operands: +// dimensions with a known non-string type (including the time dimension) must be cast to VARCHAR, while string dimensions and dimensions of unknown type are matched directly. +func TestSearchDruidSQLCastsNonStringDimensions(t *testing.T) { + mv := &runtimev1.MetricsViewSpec{ + Table: "events", + TimeDimension: "__time", + Dimensions: []*runtimev1.MetricsViewSpec_Dimension{ + {Name: "__time", Column: "__time", DataType: &runtimev1.Type{Code: runtimev1.Type_CODE_TIMESTAMP}}, + {Name: "account_id", Column: "account_id", DataType: &runtimev1.Type{Code: runtimev1.Type_CODE_INT64}}, + {Name: "account_name", Column: "account_name", DataType: &runtimev1.Type{Code: runtimev1.Type_CODE_STRING}}, + {Name: "domain", Column: "domain"}, + }, + } + + cases := map[string]string{ + "__time": `SELECT ("__time") AS "__time" FROM "events" WHERE (REGEXP_LIKE(CAST(("__time") AS VARCHAR), ?)) GROUP BY 1`, + "account_id": `SELECT ("account_id") AS "account_id" FROM "events" WHERE (REGEXP_LIKE(CAST(("account_id") AS VARCHAR), ?)) GROUP BY 1`, + "account_name": `SELECT ("account_name") AS "account_name" FROM "events" WHERE (REGEXP_LIKE(("account_name"), ?)) GROUP BY 1`, + "domain": `SELECT ("domain") AS "domain" FROM "events" WHERE (REGEXP_LIKE(("domain"), ?)) GROUP BY 1`, + } + for dim, want := range cases { + t.Run(dim, func(t *testing.T) { + qry := &metricsview.Query{ + MetricsView: "mv", + Dimensions: []metricsview.Dimension{{Name: dim}}, + Where: &metricsview.Expression{Condition: &metricsview.Condition{ + Operator: metricsview.OperatorIlike, + Expressions: []*metricsview.Expression{{Name: dim}, {Value: "%tvc%"}}, + }}, + } + ast, err := metricsview.NewAST(mv, runtime.ResolvedSecurityOpen, qry, druid.DialectDruid) + require.NoError(t, err) + + sql, args, err := ast.SQL() + require.NoError(t, err) + require.Equal(t, want, sql) + require.Equal(t, []any{"^(?i).*tvc.*$"}, args) + }) + } +} diff --git a/web-common/src/components/search/Search.svelte b/web-common/src/components/search/Search.svelte index c16e5e6dbbb9..e461a575f14c 100644 --- a/web-common/src/components/search/Search.svelte +++ b/web-common/src/components/search/Search.svelte @@ -29,7 +29,8 @@ let ref: HTMLInputElement | HTMLTextAreaElement; function handleKeyDown(event) { - if (event.code == "Enter") { + // Use `key` rather than `code`: the numeric keypad Enter reports code "NumpadEnter", and without this the form submits natively and reloads the page. + if (event.key === "Enter") { event.preventDefault(); event.stopPropagation(); onSubmit(); diff --git a/web-common/src/features/dashboards/dimension-search/useDimensionSearchResults.ts b/web-common/src/features/dashboards/dimension-search/useDimensionSearchResults.ts index 79100ea785a0..e46c8cc744d3 100644 --- a/web-common/src/features/dashboards/dimension-search/useDimensionSearchResults.ts +++ b/web-common/src/features/dashboards/dimension-search/useDimensionSearchResults.ts @@ -1,6 +1,7 @@ import { createBatches } from "@rilldata/web-common/lib/arrayUtils"; import { createQueryServiceMetricsViewSearch, + MetricsViewSpecDimensionType, type V1MetricsViewSpec, type V1TimeRangeSummary, } from "@rilldata/web-common/runtime-client"; @@ -27,7 +28,10 @@ export function useDimensionSearchResults( timeRangeSummary: V1TimeRangeSummary, searchText: string, ) { - const dimensions = metricsView.dimensions ?? []; + // Time dimensions (including the auto-added `timeseries` column) are not searchable text, so skip them. + const dimensions = (metricsView.dimensions ?? []).filter( + (d) => d.type !== MetricsViewSpecDimensionType.DIMENSION_TYPE_TIME, + ); const batches = createBatches(dimensions, BatchSize); return derived( batches.map((batch) => diff --git a/web-common/src/features/explores/ExplorePreviewCTAs.svelte b/web-common/src/features/explores/ExplorePreviewCTAs.svelte index 37555e7bb3c5..ebc75bd1eaec 100644 --- a/web-common/src/features/explores/ExplorePreviewCTAs.svelte +++ b/web-common/src/features/explores/ExplorePreviewCTAs.svelte @@ -43,14 +43,19 @@ {#if $explorePolicyCheck.data || $metricsPolicyCheck.data || $rillYamlPolicyCheck.data} {/if} - - {#if $dashboardChat} - - {/if} - {#if ready} - - {/if} - + + {#if metricsViewName} + {#key metricsViewName + exploreName} + + {#if $dashboardChat} + + {/if} + {#if ready} + + {/if} + + {/key} + {/if} {#if !$readOnly} {/if}