Skip to content

Commit

Permalink
allow mixed grouping for custom averages
Browse files Browse the repository at this point in the history
In the past this wasn't permitted because binary operations
had to have the same grouping on both sides. In Netflix#1244 that
was changed so that one side could have a subset of the other.
  • Loading branch information
brharrington committed Apr 15, 2022
1 parent d6d281c commit 3ab54e6
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -165,17 +165,13 @@ object CustomVocabulary {
val nq = extractCommonQuery(q)
val numerator = DataExpr.Sum(q)
val denominator = DataExpr.Sum(baseQuery.and(nq))
if (ks.forall(keys.contains)) {
// All keys in group by are shared by both sides
MathExpr.Divide(DataExpr.GroupBy(numerator, ks), DataExpr.GroupBy(denominator, ks))
} else if (ks.forall(k => !keys.contains(k))) {
// All keys in group by are only valid for the numerator
val denominatorKeys = ks.filter(keys.contains)
if (denominatorKeys.isEmpty) {
MathExpr.Divide(DataExpr.GroupBy(numerator, ks), denominator)
} else {
// Mix of keys that are common to both sides and keys that can only be
// applied to the numerator
throw new IllegalArgumentException(
s"invalid key list for grouping $name, mixes shared and non-shared tag keys"
MathExpr.Divide(
DataExpr.GroupBy(numerator, ks),
DataExpr.GroupBy(denominator, denominatorKeys)
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,13 @@ class CustomVocabularySuite extends FunSuite {
}

test("group by mixed keys") {
intercept[IllegalArgumentException] {
eval("name,(,a,b,c,),:in,app,beacon,:eq,:and,:node-avg,(,name,asg,),:by")
val expr = eval("name,(,a,b,c,),:in,app,beacon,:eq,:and,:node-avg,(,name,asg,),:by").rewrite {
case MathExpr.NamedRewrite("node-avg", _, e, _, _) => e
}
val expected = eval(
s"name,(,a,b,c,),:in,:sum,(,name,asg,),:by,$numInstances,:sum,(,asg,),:by,:div,app,beacon,:eq,:cq"
)
assertEquals(expr, expected)
}

test("cg after style") {
Expand Down

0 comments on commit 3ab54e6

Please sign in to comment.