Merge pull request #765 from ryankwagner/fixCandidateCalc

Add test and modification to bestCandidate Calculation
yahoo · Mar 19, 2021 · 955c900 · 955c900
2 parents 1eab70a + 78462ee
commit 955c900
Show file tree

Hide file tree

Showing 18 changed files with 101 additions and 28 deletions.
diff --git a/api-example/pom.xml b/api-example/pom.xml
@@ -11,7 +11,7 @@
     <parent>
         <artifactId>maha-parent</artifactId>
         <groupId>com.yahoo.maha</groupId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha api-example</name>

diff --git a/api-jersey/pom.xml b/api-jersey/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha api-jersey</name>

diff --git a/bigquery/pom.xml b/bigquery/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha bigquery executor</name>

diff --git a/core/pom.xml b/core/pom.xml
@@ -8,7 +8,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha core</name>

diff --git a/core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala b/core/src/main/scala/com/yahoo/maha/core/query/QueryPipeline.scala
@@ -450,22 +450,36 @@ trait QueryPipelineFactory {
 
 }
 
+case class RollupTuple(name: String, engine: Engine, costEstimate: Long, level: Int, cardinality: Int) {
+  def getTuple = (name, engine, costEstimate, level, cardinality)
+}
+
 object DefaultQueryPipelineFactory extends Logging {
 
   val druidMultiQueryEngineList: Seq[Engine] = List(OracleEngine, PostgresEngine)
 
-  private[this] def aLessThanBByLevelAndCostAndCardinality(a: (String, Engine, Long, Int, Int), b: (String, Engine, Long, Int, Int)): Boolean = {
-    if (a._2 == b._2) {
-      if (a._4 == b._4) {
-        a._3 < b._3
+  def rollupComparator(
+                        f: (RollupTuple, RollupTuple) => Boolean = aLessThanBByLevelAndCostAndCardinality
+                      ) = {
+    f
+  }
+
+  private[this] def aLessThanBByLevelAndCostAndCardinality(a: RollupTuple, b: RollupTuple): Boolean = {
+    if (a.engine == b.engine) {
+      if (a.costEstimate == b.costEstimate) {
+        a.level < b.level
       } else {
-        a._4 < b._4
+        a.costEstimate < b.costEstimate
       }
     } else {
-      if (a._5 == b._5) {
-        a._3 < b._3
+      if (a.cardinality == b.cardinality) {
+        if(a.costEstimate == b.costEstimate){
+          a.level < b.level
+        } else {
+          a.costEstimate < b.costEstimate
+        }
       } else {
-        a._5 < b._5
+        a.cardinality < b.cardinality
       }
     }
   }
@@ -529,7 +543,7 @@ object DefaultQueryPipelineFactory extends Logging {
         if (requestModel.isDebugEnabled) {
           info(s"disqualifySet = $disqualifySet")
         }
-        val result: IndexedSeq[(String, Engine, Long, Int, Int)] = requestModel.factCost.toIndexedSeq.collect {
+        val result: IndexedSeq[RollupTuple] = requestModel.factCost.toIndexedSeq.collect {
           case ((fn, engine), rowcost) if (!queryGeneratorRegistry.getDefaultGenerator(engine).isDefined || queryGeneratorRegistry.getDefaultGenerator(engine).get.validateEngineConstraints(requestModel)) && (forceEngine.contains(engine) || (!disqualifySet(engine) && forceEngine.isEmpty)) =>
             val fact = requestModel.bestCandidates.get.facts(fn).fact
             val level = fact.level
@@ -538,11 +552,11 @@ object DefaultQueryPipelineFactory extends Logging {
             if (requestModel.isDebugEnabled) {
               info(s"fn=$fn engine=$engine cost=${rowcost.costEstimate} level=$level cardinalityPreference=$dimCardinalityPreference")
             }
-            (fn, engine, rowcost.costEstimate, level, dimCardinalityPreference)
-        }.sortWith(aLessThanBByLevelAndCostAndCardinality)
+            RollupTuple(fn, engine, rowcost.costEstimate, level, dimCardinalityPreference)
+        }.sortWith(rollupComparator())
         require(result.nonEmpty,
           s"Failed to find best candidate, forceEngine=$forceEngine, engine disqualifyingSet=$disqualifySet, candidates=${requestModel.bestCandidates.get.facts.mapValues(_.fact.engine).toSet}")
-        requestModel.bestCandidates.get.getFactBestCandidate(result.head._1, requestModel)
+        requestModel.bestCandidates.get.getFactBestCandidate(result.head.name, requestModel)
     }
   }
 

diff --git a/core/src/test/scala/com/yahoo/maha/core/query/QueryPipelineWithFallbackTest.scala b/core/src/test/scala/com/yahoo/maha/core/query/QueryPipelineWithFallbackTest.scala
@@ -223,4 +223,63 @@ class QueryPipelineWithFallbackTest extends AnyFunSuite with Matchers with Befor
     pipeline = builder._1.toOption.get.build()
     assert(pipeline.fallbackQueryChainOption.isEmpty, s"No fallback query expected: $pipeline")
   }
+
+  test("Demonstrate the fix in the new result comparator.") {
+
+    def aLessThanBByLevelAndCostAndCardinality(a: RollupTuple, b: RollupTuple): Boolean = {
+      if (a.engine == b.engine) {
+        if (a.level == b.level) {
+          a.costEstimate < b.costEstimate
+        } else {
+          a.level < b.level
+        }
+      } else {
+        if (a.cardinality == b.cardinality) {
+          a.costEstimate < b.costEstimate
+        } else {
+          a.cardinality < b.cardinality
+        }
+      }
+    }
+
+    //(fn, engine, rowcost.costEstimate, level, dimCardinalityPreference)
+    //(name:String, engine: Engine, cost: Long, level: Int, cardinality: Int)
+
+    val t1 = RollupTuple("dr_stats_hourly", DruidEngine, 1600L, 9993, 8675309)
+    val t2 = RollupTuple("dr_ad_stats_hourly", DruidEngine, 1600L, 9995, 8675309)
+    val t3 = RollupTuple("oracle_stats", OracleEngine, 1600L, 9992, 8675309)
+    val t4 = RollupTuple("oracle_ad_stats", OracleEngine, 1600L, 9994, 8675309)
+    val t5 = RollupTuple("dr_teacher_ad_stats", DruidEngine, 1600L, 9998, 8675309)
+    val t6 = RollupTuple("oracle_teacher_stats", OracleEngine, 1600L, 9991, 8675309)
+    val t7 = RollupTuple("dr_teacher_stats_hourly", DruidEngine, 1600L, 9992, 8675309)
+    val t8 = RollupTuple("dr_teacher_ad_stats_hourly", DruidEngine, 1600L, 9997, 8675309)
+
+
+    val newResult = Vector(t1,t2,t3,t4,t5,t6,t7,t8).sortWith(DefaultQueryPipelineFactory.rollupComparator())
+    val oldResult = Vector(t1,t2,t3,t4,t5,t6,t7,t8).sortWith(DefaultQueryPipelineFactory.rollupComparator(aLessThanBByLevelAndCostAndCardinality))
+
+    //println(oldResult.mkString("\n"))
+    //println(newResult.mkString("\n"))
+
+    assert(oldResult.map(_.getTuple).mkString("\n").equals(
+      "(dr_stats_hourly,Druid,1600,9993,8675309)\n" +
+        "(dr_ad_stats_hourly,Druid,1600,9995,8675309)\n" +
+        "(oracle_stats,Oracle,1600,9992,8675309)\n" +
+        "(oracle_ad_stats,Oracle,1600,9994,8675309)\n" +
+        "(dr_teacher_ad_stats,Druid,1600,9998,8675309)\n" +
+        "(oracle_teacher_stats,Oracle,1600,9991,8675309)\n" +
+        "(dr_teacher_stats_hourly,Druid,1600,9992,8675309)\n" +
+        "(dr_teacher_ad_stats_hourly,Druid,1600,9997,8675309)"
+    ))
+    assert(newResult.map(_.getTuple).mkString("\n").equals(
+      "(oracle_teacher_stats,Oracle,1600,9991,8675309)\n" +
+        "(oracle_stats,Oracle,1600,9992,8675309)\n" +
+        "(dr_teacher_stats_hourly,Druid,1600,9992,8675309)\n" +
+        "(dr_stats_hourly,Druid,1600,9993,8675309)\n" +
+        "(oracle_ad_stats,Oracle,1600,9994,8675309)\n" +
+        "(dr_ad_stats_hourly,Druid,1600,9995,8675309)\n" +
+        "(dr_teacher_ad_stats_hourly,Druid,1600,9997,8675309)\n" +
+        "(dr_teacher_ad_stats,Druid,1600,9998,8675309)"
+    ))
+  }
 }
diff --git a/db/pom.xml b/db/pom.xml
@@ -8,7 +8,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha db</name>

diff --git a/druid-lookups/pom.xml b/druid-lookups/pom.xml
@@ -5,7 +5,7 @@
     <parent>
         <artifactId>maha-parent</artifactId>
         <groupId>com.yahoo.maha</groupId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
     <modelVersion>4.0.0</modelVersion>
 

diff --git a/druid/pom.xml b/druid/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha druid executor</name>

diff --git a/job-service/pom.xml b/job-service/pom.xml
@@ -8,7 +8,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha job service</name>

diff --git a/oracle/pom.xml b/oracle/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha oracle executor</name>

diff --git a/par-request-2/pom.xml b/par-request-2/pom.xml
@@ -9,7 +9,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
 

diff --git a/pom.xml b/pom.xml
@@ -5,7 +5,7 @@
 
     <groupId>com.yahoo.maha</groupId>
     <artifactId>maha-parent</artifactId>
-    <version>6.37-SNAPSHOT</version>
+    <version>6.38-SNAPSHOT</version>
     <packaging>pom</packaging>
 
     <name>maha parent</name>

diff --git a/postgres/pom.xml b/postgres/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha postgres executor</name>

diff --git a/presto/pom.xml b/presto/pom.xml
@@ -10,7 +10,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha presto executor</name>

diff --git a/request-log/pom.xml b/request-log/pom.xml
@@ -8,7 +8,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha request log</name>

diff --git a/service/pom.xml b/service/pom.xml
@@ -9,7 +9,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha service</name>

diff --git a/worker/pom.xml b/worker/pom.xml
@@ -8,7 +8,7 @@
     <parent>
         <groupId>com.yahoo.maha</groupId>
         <artifactId>maha-parent</artifactId>
-        <version>6.37-SNAPSHOT</version>
+        <version>6.38-SNAPSHOT</version>
     </parent>
 
     <name>maha worker</name>