update normalize behavior for UI (#1474)

Updates the behavior of normalize to reflect preferences
of the UI team. In particular:

- It will not try to extract common query aspects into
  a `:cq` clause.
- The helpers for filtering based on summary stats will
  be preserved rather than expanding.
- Adjust handling of OR clauses in a query. First convert
  to DNF and then sort the CNF clauses to avoid awkward
  expansion with repeated terms.

atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala

@@ -24,9 +24,21 @@ trait FilterExpr extends TimeSeriesExpr
 
 object FilterExpr {
 
-  case class Stat(expr: TimeSeriesExpr, stat: String) extends FilterExpr {
+  /**
+    * Represents a basic summary stat for an input time series.
+    *
+    * @param expr
+    *     Input expression to compute the stat over.
+    * @param stat
+    *     Which summary stat to compute.
+    * @param str
+    *     Optional string representation of the expression. Used for common-cases of helper
+    *     functions to avoid duplication of the original expression.
+    */
+  case class Stat(expr: TimeSeriesExpr, stat: String, str: Option[String] = None)
+      extends FilterExpr {
 
-    override def toString: String = s"$expr,$stat,:stat"
+    override def toString: String = str.getOrElse(s"$expr,$stat,:stat")
 
     def dataExprs: List[DataExpr] = expr.dataExprs
 

atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterVocabulary.scala

@@ -220,7 +220,7 @@ object FilterVocabulary extends Vocabulary {
 
     private def rewriteStatExprs(t1: TimeSeriesExpr, t2: TimeSeriesExpr): TimeSeriesExpr = {
       val r = t2.rewrite {
-        case s: FilterExpr.StatExpr => FilterExpr.Stat(t1, s.name)
+        case s: FilterExpr.StatExpr => FilterExpr.Stat(t1, s.name, Some(s.toString))
       }
       r.asInstanceOf[TimeSeriesExpr]
     }

atlas-core/src/test/scala/com/netflix/atlas/core/model/FilterSuite.scala

@@ -15,6 +15,7 @@
  */
 package com.netflix.atlas.core.model
 
+import com.netflix.atlas.core.stacklang.Interpreter
 import munit.FunSuite
 
 class FilterSuite extends FunSuite {
@@ -57,4 +58,30 @@ class FilterSuite extends FunSuite {
     val expr = MathExpr.Add(filteredExpr, filteredExpr)
     assert(expr.eval(context, Nil).data.isEmpty)
   }
+
+  private val interpreter = Interpreter(FilterVocabulary.allWords)
+
+  private def parse(str: String): TimeSeriesExpr = {
+    interpreter.execute(str).stack match {
+      case ModelExtractors.TimeSeriesType(t) :: Nil => t
+      case _                                        => throw new MatchError(str)
+    }
+  }
+
+  test("toString for max,:stat") {
+    val expr =
+      "name,sps,:eq,:sum,(,app,),:by,name,sps,:eq,:sum,(,app,),:by,max,:stat,5.0,:const,:gt,:filter"
+    assertEquals(parse(expr).toString, expr)
+  }
+
+  test("toString for stat-max") {
+    val expr = "name,sps,:eq,:sum,(,app,),:by,:stat-max,5.0,:const,:gt,:filter"
+    assertEquals(parse(expr).toString, expr)
+  }
+
+  test("toString for several stat-aggr uses") {
+    val filter = ":stat-max,:stat-avg,5.0,:const,:add,:div,:stat-min,:gt"
+    val expr = s"name,sps,:eq,:sum,(,app,),:by,$filter,:filter"
+    assertEquals(parse(expr).toString, expr)
+  }
 }

atlas-webapi/src/main/scala/com/netflix/atlas/webapi/ExprApi.scala

@@ -23,14 +23,11 @@ import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server.Route
 import com.netflix.atlas.akka.CustomDirectives._
 import com.netflix.atlas.akka.WebApi
-import com.netflix.atlas.core.model.DataExpr
 import com.netflix.atlas.core.model.Expr
 import com.netflix.atlas.core.model.FilterExpr
-import com.netflix.atlas.core.model.MathExpr
 import com.netflix.atlas.core.model.ModelExtractors
 import com.netflix.atlas.core.model.Query
 import com.netflix.atlas.core.model.StyleExpr
-import com.netflix.atlas.core.model.TimeSeriesExpr
 import com.netflix.atlas.core.stacklang.Context
 import com.netflix.atlas.core.stacklang.Interpreter
 import com.netflix.atlas.core.stacklang.Word
@@ -94,8 +91,8 @@ class ExprApi extends WebApi {
       case "query" =>
         // Expectation is that there would be a single query on the stack
         stack match {
-          case v :: Nil =>
-          case v :: vs =>
+          case _ :: Nil =>
+          case _ :: _ =>
             val summary = Interpreter.typeSummary(stack)
             throw new IllegalArgumentException(s"expected a single query, found $summary")
           case Nil =>
@@ -243,7 +240,7 @@ class ExprApi extends WebApi {
 
   private def stripFilter(expr: Expr): Expr = {
     expr.rewrite {
-      case FilterExpr.Stat(e, _)            => e
+      case FilterExpr.Stat(e, _, _)         => e
       case FilterExpr.Filter(e, _)          => e
       case e: FilterExpr.PriorityFilterExpr => e.expr
     }
@@ -271,53 +268,14 @@ object ExprApi {
   }
 
   def normalize(exprs: List[StyleExpr]): List[String] = {
-
-    // If named rewrites are used, then map the eval expression to match the display
-    // expression. This avoids the complexity of the eval expression showing up in the
-    // extracted data expressions.
-    import MathExpr.NamedRewrite
-    val cleaned = exprs.map { e =>
-      val clean = e.rewrite {
-        case r @ NamedRewrite(_, orig: Query, _, _, _) =>
-          r.copy(evalExpr = DataExpr.Sum(orig))
-        case r @ NamedRewrite(_, orig: TimeSeriesExpr, _, _, _) =>
-          r.copy(evalExpr = orig)
-      }
-      clean.asInstanceOf[StyleExpr]
-    }
-
-    // Extract the distinct queries from all data expressions
-    val queries = cleaned.flatMap(_.expr.dataExprs.map(_.query)).distinct
-
-    // Map from original query to CNF form. This is used to find the common clauses that
-    // can be extracted and applied to the overall query using :cq.
-    val cnfQueries = queries.map(q => q -> Query.cnfList(q).toSet).toMap
-
-    // Find the set of common query clauses
-    val commonQueries =
-      if (cnfQueries.isEmpty) Set.empty[Query]
-      else
-        cnfQueries.values.reduceLeft { (s1, s2) =>
-          s1.intersect(s2)
-        }
-
     // Normalize the legend vars
     val styleExprs = exprs.map(normalizeLegendVars)
 
-    // Normalize without extracting common queries
-    val normalized = exprStrings(styleExprs, cnfQueries, Set.empty)
-    val fullExpr = normalized.mkString(",")
-
-    // Normalize with extracting common queries
-    val cq = s":list,(,${sort(commonQueries, Set.empty)},:cq,),:each"
-    val normalizedCQ = cq :: exprStrings(styleExprs, cnfQueries, commonQueries)
-    val fullExprCQ = normalizedCQ.mkString(",")
-
-    // If needed, then prepend a common query conversion to the list
-    val finalList = if (fullExpr.length < fullExprCQ.length) normalized else normalizedCQ
+    // Normalized expression strings
+    val normalized = exprStrings(styleExprs)
 
     // Reverse the list to match the order the user would expect
-    finalList.reverse
+    normalized.reverse
   }
 
   // For use-cases such as performing automated rewrites of expressions to move off of legacy
@@ -337,19 +295,11 @@ object ExprApi {
     }
   }
 
-  private def exprStrings(
-    exprs: List[StyleExpr],
-    cnf: Map[Query, Set[Query]],
-    cq: Set[Query]
-  ): List[String] = {
-
-    // Map from original query to sorted query without the common clauses excluded
-    val sortedQueries = cnf.map { case (q, qs) => q -> sort(qs, cq) }
-
+  private def exprStrings(exprs: List[StyleExpr]): List[String] = {
     // Rewrite the expressions and convert to a normalized strings
     exprs.map { expr =>
       val rewritten = expr.rewrite {
-        case q: Query => sortedQueries.getOrElse(q, q)
+        case q: Query => sort(q)
       }
       // Remove explicit :const, it can be determined from implicit conversion
       // and adds visual clutter
@@ -364,13 +314,18 @@ object ExprApi {
     * will have an equal result query even if they were in different orders in the input
     * expression string.
     */
-  private def sort(qs: Set[Query], exclude: Set[Query]): Query = {
-    val matches = qs.toList.filter(q => !exclude.contains(q))
-    if (matches.isEmpty)
-      Query.True
-    else
-      matches.sortWith(_.toString < _.toString).reduce { (q1, q2) =>
-        Query.And(q1, q2)
+  private def sort(query: Query): Query = {
+    val simplified = Query.simplify(query)
+    Query
+      .dnfList(simplified)
+      .map { q =>
+        Query.cnfList(q).sortWith(_.toString < _.toString).reduce { (q1, q2) =>
+          Query.And(q1, q2)
+        }
+      }
+      .sortWith(_.toString < _.toString) // order OR clauses
+      .reduce { (q1, q2) =>
+        Query.Or(q1, q2)
       }
   }
 }

atlas-webapi/src/test/scala/com/netflix/atlas/webapi/ExprApiSuite.scala

@@ -32,7 +32,7 @@ class ExprApiSuite extends MUnitRouteSuite {
 
   import scala.concurrent.duration._
 
-  implicit val routeTestTimeout = RouteTestTimeout(5.second)
+  private implicit val routeTestTimeout: RouteTestTimeout = RouteTestTimeout(5.second)
 
   val endpoint = new ExprApi
 
@@ -118,8 +118,10 @@ class ExprApiSuite extends MUnitRouteSuite {
   ) {
     assertEquals(response.status, StatusCodes.OK)
     val data = Json.decode[List[String]](responseAs[String])
-    val expected =
-      List("name,cpu,:eq,:sum", "name,disk,:eq,:sum", ":list,(,nf.cluster,foo,:eq,:cq,),:each")
+    val expected = List(
+      "name,cpu,:eq,nf.cluster,foo,:eq,:and,:sum",
+      "name,disk,:eq,nf.cluster,foo,:eq,:and,:sum"
+    )
     assertEquals(data, expected)
   }
 
@@ -246,7 +248,7 @@ class ExprApiSuite extends MUnitRouteSuite {
   testGet("/api/v1/expr/strip?q=name,sps,:eq,:stat-max,5,:gt,:filter&r=style") {
     assertEquals(response.status, StatusCodes.OK)
     val data = Json.decode[List[String]](responseAs[String])
-    assertEquals(data, List("name,sps,:eq,:sum,name,sps,:eq,:sum,max,:stat,5.0,:const,:gt,:filter"))
+    assertEquals(data, List("name,sps,:eq,:sum,:stat-max,5.0,:const,:gt,:filter"))
   }
 
   testGet(
@@ -300,21 +302,21 @@ class ExprApiSuite extends MUnitRouteSuite {
     assertEquals(
       normalize(add.toString),
       List(
-        ":true,:sum,:true,:sum,:add",
-        ":list,(,app,foo,:eq,name,cpuUser,:eq,:and,:cq,),:each"
+        "app,foo,:eq,name,cpuUser,:eq,:and,:sum,app,foo,:eq,name,cpuUser,:eq,:and,:sum,:add"
       )
     )
   }
 
   test("normalize common query") {
+    // No longer performed, UI team prefers not having the cq in the
+    // normalized query
     val e1 = DataExpr.Sum(And(Equal("app", "foo"), Equal("name", "cpuUser")))
     val e2 = DataExpr.Sum(And(Equal("app", "foo"), Equal("name", "cpuSystem")))
     val add = StyleExpr(MathExpr.Add(e1, e2), Map.empty)
     assertEquals(
       normalize(add.toString),
       List(
-        "name,cpuUser,:eq,:sum,name,cpuSystem,:eq,:sum,:add",
-        ":list,(,app,foo,:eq,:cq,),:each"
+        "app,foo,:eq,name,cpuUser,:eq,:and,:sum,app,foo,:eq,name,cpuSystem,:eq,:and,:sum,:add"
       )
     )
   }
@@ -350,9 +352,8 @@ class ExprApiSuite extends MUnitRouteSuite {
     assertEquals(
       normalize(avg),
       List(
-        "name,cpuUser,:eq,:dist-avg",
-        "name,cpuSystem,:eq,:max",
-        ":list,(,app,foo,:eq,:cq,),:each"
+        "app,foo,:eq,name,cpuUser,:eq,:and,:dist-avg",
+        "app,foo,:eq,name,cpuSystem,:eq,:and,:max"
       )
     )
   }
@@ -362,6 +363,29 @@ class ExprApiSuite extends MUnitRouteSuite {
     assertEquals(normalize(avg), List(avg))
   }
 
+  test("normalize :stat-aggr filters") {
+    val avg = "app,foo,:eq,name,cpuUser,:eq,:and,:sum,(,nf.cluster,),:by,:stat-max,5.0,:gt,:filter"
+    assertEquals(normalize(avg), List(avg))
+  }
+
+  test("normalize simplify query") {
+    val input = "app,foo,:eq,name,cpuUser,:eq,:and,:true,:and,:sum"
+    val expected = "app,foo,:eq,name,cpuUser,:eq,:and,:sum"
+    assertEquals(normalize(input), List(expected))
+  }
+
+  test("normalize sort query") {
+    val input = "name,cpuUser,:eq,app,foo,:eq,:and,:sum"
+    val expected = "app,foo,:eq,name,cpuUser,:eq,:and,:sum"
+    assertEquals(normalize(input), List(expected))
+  }
+
+  test("normalize sensible OR handling") {
+    val input = "name,cpuUser,:eq,app,foo,:eq,:and,name,cpuUser2,:eq,app,bar,:eq,:and,:or,:sum"
+    val expected = "app,bar,:eq,name,cpuUser2,:eq,:and,app,foo,:eq,name,cpuUser,:eq,:and,:or,:sum"
+    assertEquals(normalize(input), List(expected))
+  }
+
   test("normalize :des-fast") {
     val expr = "app,foo,:eq,name,cpuUser,:eq,:and,:sum,:des-fast"
     assertEquals(normalize(expr), List(expr))