Merge pull request #1550 from sandeep6189/fix_preagg_query_parsing

fix(query): Updating Regex to support preagg metric names with column selector

prometheus/src/main/scala/filodb/prometheus/ast/Vectors.scala

@@ -135,7 +135,11 @@ sealed trait Vector extends Expression {
 
   def metricName: Option[String]
   def labelSelection: Seq[LabelMatch]
-  val regexColumnName: String = "::(?=[^::]+$)" //regex pattern to extract ::columnName at the end
+
+  // UPDATE April 04, 2023: Updating the regex to split only on "::" and not on ":::". The metric names might have
+  // ":::" for certain cases, like the aggregated metrics from streaming aggregation. For ex -
+  // "requests:::rule_1::sum" should give -> ("requests:::rule_1", "sum") on split
+  val regexColumnName: String = "(?=.)+(?<!:)::(?=[^:]+$)" //regex pattern to extract ::columnName at the end
 
   // Convert metricName{labels} -> {labels, __name__="metricName"} so it's uniform
   // Note: see makeMergeNameToLabels before changing the laziness.
@@ -199,10 +203,11 @@ sealed trait Vector extends Expression {
   def realMetricName: String = mergeNameToLabels.find(_.label == PromMetricLabel).get.value
 
   // Returns (trimmedMetricName, column) after stripping ::columnName
-  private def extractStripColumn(metricName: String): (String, Option[String]) = {
+  protected def extractStripColumn(metricName: String): (String, Option[String]) = {
     val parts = metricName.split(regexColumnName)
     if (parts.size > 1) {
       require(parts(1).nonEmpty, "cannot use empty column name")
+      require(!parts(1).isBlank, "cannot use blank/whitespace column name")
       (parts(0), Some(parts(1)))
     } else (metricName, None)
   }
@@ -247,6 +252,21 @@ sealed trait Vector extends Expression {
   }
 }
 
+/**
+ * Adding a class to be able to unit test few methods
+ * of the sealed trait Vector like "extractStripColumn"
+ */
+case class VectorSpec() extends Vector{
+  override def labelSelection: Seq[LabelMatch] = Seq.empty
+  override def metricName: Option[String] = None
+
+  // helper function created to unit test "extractStripColumn" function in sealted trait Vector
+  def getMetricAndColumnName(metricName: String): (String, Option[String]) = {
+    extractStripColumn(metricName)
+  }
+  override def applyPromqlConstraints(): Unit = {}
+}
+
 /**
   * Instant vector selectors allow the selection of a set of time series
   * and a single sample value for each at a given timestamp (instant):

prometheus/src/test/scala/filodb/prometheus/parse/ParserSpec.scala

@@ -1,12 +1,11 @@
 package filodb.prometheus.parse
 
-import filodb.prometheus.ast.{PeriodicSeries, RangeExpression, SimpleSeries, SubqueryClause, SubqueryExpression, TimeStepParams}
+import filodb.prometheus.ast.{PeriodicSeries, RangeExpression, SimpleSeries, SubqueryClause, SubqueryExpression, TimeStepParams, VectorSpec}
 import filodb.prometheus.parse.Parser.{Antlr, Shadow}
 import filodb.query.{BinaryJoin, LogicalPlan}
 import org.scalatest.funspec.AnyFunSpec
 import org.scalatest.matchers.should.Matchers
 
-
 import java.util.concurrent.TimeUnit.{MINUTES, SECONDS, HOURS, DAYS}
 import scala.concurrent.duration.Duration
 
@@ -19,6 +18,39 @@ class ParserSpec extends AnyFunSpec with Matchers {
     parseSuccessfully("http_requests_total{job=\"prometheus\", method=\"GET\"} limit 1")
   }
 
+  it("test columnName split regex for different type of metric names") {
+
+    // Various test cases to test the regex on
+    val input = List(
+      ("http_requests_total:::aggregated_rule","http_requests_total:::aggregated_rule", None),
+      ("http_requests_total:::aggregated_rule::count","http_requests_total:::aggregated_rule", Some("count")),
+      ("http_requests_total::sum","http_requests_total", Some("sum")),
+      ("http_requests_total","http_requests_total", None),
+      ("mygauge","mygauge", None),
+      ("mygauge:sum","mygauge:sum", None),
+      ("mygauge:sum:::","mygauge:sum:::", None),
+    )
+
+    input.foreach { i => {
+      val (metricName, columnName) = VectorSpec.apply().getMetricAndColumnName(i._1)
+      metricName shouldEqual i._2
+      columnName shouldEqual i._3
+    }}
+  }
+
+  it("test columnName split regex should throw IllegalArgumentException for invalid metric name") {
+
+    val input = List(
+      ("http_requests_total:: ", "cannot use blank/whitespace column name"),
+      ("test::    ", "cannot use blank/whitespace column name"),
+    )
+
+    input.foreach { i => {
+      val thrownEx = the [IllegalArgumentException] thrownBy(VectorSpec.apply().getMetricAndColumnName(i._1))
+      thrownEx.getMessage.contains(i._2) shouldEqual true
+    }}
+  }
+
   it("metadata matcher query") {
     parseSuccessfully("http_requests_total{job=\"prometheus\", method=\"GET\"}")
     parseSuccessfully("http_requests_total{job=\"prometheus\", method=\"GET\"}")