Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Adding support for defining filters on measures #3624

Merged
merged 26 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2f56587
Initial commit
AdityaHegde Dec 5, 2023
102ac29
Using enum for expression and adding to metricsview_aggregation
AdityaHegde Dec 6, 2023
ccfeb0e
Merge branch 'main' into adityahegde/filters-on-measures
AdityaHegde Dec 7, 2023
d439c54
Comparison in measure filters
AdityaHegde Dec 7, 2023
655ab2e
Adding to metricsview toplist
AdityaHegde Dec 7, 2023
4808486
Updated proto spec
AdityaHegde Dec 11, 2023
74bddbd
Add generic expression syntax
AdityaHegde Dec 12, 2023
3bc151a
Final proto draft
AdityaHegde Dec 13, 2023
d0d5228
Integrate and test where filter
AdityaHegde Dec 13, 2023
0f9c159
Integrate where clause everywhere
AdityaHegde Dec 13, 2023
171a7f2
Merge branch 'main' into adityahegde/filters-on-measures
AdityaHegde Dec 13, 2023
9d9b016
Adding some tests for having clauses
AdityaHegde Dec 13, 2023
aa1b379
Add backwards compatibility for filter
AdityaHegde Dec 14, 2023
e9ff93b
fix lint
AdityaHegde Dec 14, 2023
a828601
Adding alias support in metricsview_comparison_toplist
AdityaHegde Dec 14, 2023
5a7526a
Apply sum() only when having is enabled
AdityaHegde Dec 14, 2023
38db0ff
Fix lint
AdityaHegde Dec 14, 2023
0ddcba5
PR comments
AdityaHegde Dec 15, 2023
f02b422
Move filter helpers to pkg/expressionpb
AdityaHegde Dec 15, 2023
a4086af
refactor filter builder to not use map
AdityaHegde Dec 15, 2023
1e49865
Backwards compatibility for older sort types
AdityaHegde Dec 15, 2023
0d02580
PR comments
AdityaHegde Dec 15, 2023
025a62d
Testing on druid and fixes
AdityaHegde Dec 18, 2023
b5754b3
Merge branch 'main' into adityahegde/filters-on-measures
AdityaHegde Dec 18, 2023
d1186d1
Using outer query
AdityaHegde Dec 18, 2023
1832ebf
Removing unused testdata
AdityaHegde Dec 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 65 additions & 48 deletions runtime/queries/metricsview.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"google.golang.org/protobuf/types/known/structpb"
)

var emptyMeasureAliases = make([]*runtimev1.MetricsViewComparisonMeasureAlias, 0)
AdityaHegde marked this conversation as resolved.
Show resolved Hide resolved

// resolveMeasures returns the selected measures
func resolveMeasures(mv *runtimev1.MetricsViewSpec, inlines []*runtimev1.InlineMeasure, selectedNames []string) ([]*runtimev1.MetricsViewSpec_MeasureV2, error) {
// Build combined measures
Expand Down Expand Up @@ -293,28 +295,54 @@ func buildFilterClauseForCondition(mv *runtimev1.MetricsViewSpec, cond *runtimev
return fmt.Sprintf("AND (%s) ", condsClause), args, nil
}

type columnIdentifier struct {
// expression to use instead of a name for dimension or expression
// EG: measure expression : impressions => "impressions" (would be aliases in query)
// dimension column : publisher => "publisher"
// dimension expression : tld => "regexp_extract(domain, '(.*\\.)?(.*\\.com)', 2)" (needed since tld might not be selected)
expr string
unnest bool
}
func columnIdentifierExpression(mv *runtimev1.MetricsViewSpec, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, name string) (string, bool) {
// check if identifier is a dimension
for _, dim := range mv.Dimensions {
if dim.Name == name {
return safeName(metricsViewDimensionColumn(dim)), true
}
}

// check if identifier is passed as an alias
for _, alias := range aliases {
if alias.Alias == name {
switch alias.Type {
case runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_BASE_VALUE,
runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_UNSPECIFIED:
return safeName(alias.Name), true
case runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_COMPARISON_VALUE:
return safeName(alias.Name + "__previous"), true
case runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_ABS_DELTA:
return safeName(alias.Name + "__delta_abs"), true
case runtimev1.MetricsViewComparisonMeasureType_METRICS_VIEW_COMPARISON_MEASURE_TYPE_REL_DELTA:
return safeName(alias.Name + "__delta_rel"), true
}
}
}

// check if identifier is measure but not passed as alias
for _, mes := range mv.Measures {
if mes.Name == name {
return safeName(mes.Name), true
}
}

func newIdentifier(name string) columnIdentifier {
return columnIdentifier{safeName(name), false}
return "", false
}

func dimensionAliases(mv *runtimev1.MetricsViewSpec) map[string]columnIdentifier {
aliases := map[string]columnIdentifier{}
for _, dim := range mv.Dimensions {
aliases[dim.Name] = columnIdentifier{safeName(metricsViewDimensionColumn(dim)), dim.Unnest}
func identifierIsUnnest(mv *runtimev1.MetricsViewSpec, expr *runtimev1.Expression) bool {
ident, isIdent := expr.Expression.(*runtimev1.Expression_Ident)
if isIdent {
for _, dim := range mv.Dimensions {
if dim.Name == ident.Ident {
return dim.Unnest
}
}
}
return aliases
return false
}

func buildExpression(expr *runtimev1.Expression, allowedIdentifiers map[string]columnIdentifier, dialect drivers.Dialect) (string, []any, error) {
func buildExpression(mv *runtimev1.MetricsViewSpec, expr *runtimev1.Expression, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, dialect drivers.Dialect) (string, []any, error) {
var emptyArg []any
switch e := expr.Expression.(type) {
case *runtimev1.Expression_Val:
Expand All @@ -325,40 +353,40 @@ func buildExpression(expr *runtimev1.Expression, allowedIdentifiers map[string]c
return "?", []any{arg}, nil

case *runtimev1.Expression_Ident:
col, ok := allowedIdentifiers[e.Ident]
if !ok {
expr, isIdent := columnIdentifierExpression(mv, aliases, e.Ident)
if !isIdent {
return "", emptyArg, fmt.Errorf("unknown column filter: %s", e.Ident)
}
return col.expr, emptyArg, nil
return expr, emptyArg, nil

case *runtimev1.Expression_Cond:
return buildConditionExpression(e.Cond, allowedIdentifiers, dialect)
return buildConditionExpression(mv, e.Cond, aliases, dialect)
}

return "", emptyArg, nil
}

func buildConditionExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]columnIdentifier, dialect drivers.Dialect) (string, []any, error) {
func buildConditionExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, dialect drivers.Dialect) (string, []any, error) {
switch cond.Op {
case runtimev1.Operation_OPERATION_LIKE, runtimev1.Operation_OPERATION_NLIKE:
return buildLikeExpression(cond, allowedIdentifiers, dialect)
return buildLikeExpression(mv, cond, aliases, dialect)

case runtimev1.Operation_OPERATION_IN, runtimev1.Operation_OPERATION_NIN:
return buildInExpression(cond, allowedIdentifiers, dialect)
return buildInExpression(mv, cond, aliases, dialect)

case runtimev1.Operation_OPERATION_AND:
return buildAndOrExpressions(cond, allowedIdentifiers, dialect, " AND ")
return buildAndOrExpressions(mv, cond, aliases, dialect, " AND ")

case runtimev1.Operation_OPERATION_OR:
return buildAndOrExpressions(cond, allowedIdentifiers, dialect, " OR ")
return buildAndOrExpressions(mv, cond, aliases, dialect, " OR ")

default:
leftExpr, args, err := buildExpression(cond.Exprs[0], allowedIdentifiers, dialect)
leftExpr, args, err := buildExpression(mv, cond.Exprs[0], aliases, dialect)
if err != nil {
return "", nil, err
}

rightExpr, subArgs, err := buildExpression(cond.Exprs[1], allowedIdentifiers, dialect)
rightExpr, subArgs, err := buildExpression(mv, cond.Exprs[1], aliases, dialect)
if err != nil {
return "", nil, err
}
Expand All @@ -368,17 +396,17 @@ func buildConditionExpression(cond *runtimev1.Condition, allowedIdentifiers map[
}
}

func buildLikeExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]columnIdentifier, dialect drivers.Dialect) (string, []any, error) {
func buildLikeExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, dialect drivers.Dialect) (string, []any, error) {
if len(cond.Exprs) != 2 {
return "", nil, fmt.Errorf("like/not like expression should have exactly 2 sub expressions")
}

leftExpr, args, err := buildExpression(cond.Exprs[0], allowedIdentifiers, dialect)
leftExpr, args, err := buildExpression(mv, cond.Exprs[0], aliases, dialect)
if err != nil {
return "", nil, err
}

rightExpr, subArgs, err := buildExpression(cond.Exprs[1], allowedIdentifiers, dialect)
rightExpr, subArgs, err := buildExpression(mv, cond.Exprs[1], aliases, dialect)
if err != nil {
return "", nil, err
}
Expand All @@ -390,12 +418,7 @@ func buildLikeExpression(cond *runtimev1.Condition, allowedIdentifiers map[strin
}

// identify if immediate identifier has unnest
unnest := false
ident, isIdent := cond.Exprs[0].Expression.(*runtimev1.Expression_Ident)
if isIdent {
i := allowedIdentifiers[ident.Ident]
unnest = i.unnest
}
unnest := identifierIsUnnest(mv, cond.Exprs[0])

var clause string
// Build [NOT] len(list_filter("dim", x -> x ILIKE ?)) > 0
Expand All @@ -419,12 +442,12 @@ func buildLikeExpression(cond *runtimev1.Condition, allowedIdentifiers map[strin
return clause, args, nil
}

func buildInExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]columnIdentifier, dialect drivers.Dialect) (string, []any, error) {
func buildInExpression(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, dialect drivers.Dialect) (string, []any, error) {
if len(cond.Exprs) <= 1 {
return "", nil, fmt.Errorf("in/not in expression should have atleast 2 sub expressions")
}

leftExpr, args, err := buildExpression(cond.Exprs[0], allowedIdentifiers, dialect)
leftExpr, args, err := buildExpression(mv, cond.Exprs[0], aliases, dialect)
if err != nil {
return "", nil, err
}
Expand All @@ -445,7 +468,7 @@ func buildInExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]
continue // Handled later using "dim IS [NOT] NULL" clause
}
}
inVal, subArgs, err := buildExpression(subExpr, allowedIdentifiers, dialect)
inVal, subArgs, err := buildExpression(mv, subExpr, aliases, dialect)
if err != nil {
return "", nil, err
}
Expand All @@ -454,13 +477,7 @@ func buildInExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]
}

// identify if immediate identifier has unnest
// TODO: do we need to do a deeper check?
unnest := false
ident, isIndent := cond.Exprs[0].Expression.(*runtimev1.Expression_Ident)
if isIndent {
i := allowedIdentifiers[ident.Ident]
unnest = i.unnest
}
unnest := identifierIsUnnest(mv, cond.Exprs[0])

clauses := make([]string, 0)

Expand Down Expand Up @@ -498,11 +515,11 @@ func buildInExpression(cond *runtimev1.Condition, allowedIdentifiers map[string]
return condsClause, args, nil
}

func buildAndOrExpressions(cond *runtimev1.Condition, allowedIdentifiers map[string]columnIdentifier, dialect drivers.Dialect, joiner string) (string, []any, error) {
func buildAndOrExpressions(mv *runtimev1.MetricsViewSpec, cond *runtimev1.Condition, aliases []*runtimev1.MetricsViewComparisonMeasureAlias, dialect drivers.Dialect, joiner string) (string, []any, error) {
clauses := make([]string, 0)
var args []any
for _, expr := range cond.Exprs {
clause, subArgs, err := buildExpression(expr, allowedIdentifiers, dialect)
clause, subArgs, err := buildExpression(mv, expr, aliases, dialect)
if err != nil {
return "", nil, err
}
Expand Down
6 changes: 2 additions & 4 deletions runtime/queries/metricsview_aggregation.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ func (q *MetricsViewAggregation) buildMetricsAggregationSQL(mv *runtimev1.Metric
args = append(args, exprArgs...)
}

measureAliases := map[string]columnIdentifier{}
for _, m := range q.Measures {
switch m.BuiltinMeasure {
case runtimev1.BuiltinMeasure_BUILTIN_MEASURE_UNSPECIFIED:
Expand All @@ -207,7 +206,6 @@ func (q *MetricsViewAggregation) buildMetricsAggregationSQL(mv *runtimev1.Metric
default:
return "", nil, fmt.Errorf("unknown builtin measure '%d'", m.BuiltinMeasure)
}
measureAliases[m.Name] = newIdentifier(m.Name)
}

groupClause := ""
Expand All @@ -225,7 +223,7 @@ func (q *MetricsViewAggregation) buildMetricsAggregationSQL(mv *runtimev1.Metric
whereClause += clause
}
if q.Where != nil {
clause, clauseArgs, err := buildExpression(q.Where, dimensionAliases(mv), dialect)
clause, clauseArgs, err := buildExpression(mv, q.Where, emptyMeasureAliases, dialect)
if err != nil {
return "", nil, err
}
Expand All @@ -240,7 +238,7 @@ func (q *MetricsViewAggregation) buildMetricsAggregationSQL(mv *runtimev1.Metric
if q.Having != nil {
var havingClauseArgs []any
var err error
havingClause, havingClauseArgs, err = buildExpression(q.Having, measureAliases, dialect)
havingClause, havingClauseArgs, err = buildExpression(mv, q.Having, emptyMeasureAliases, dialect)
if err != nil {
return "", nil, err
}
Expand Down
Loading
Loading