[SPARK-26129][SQL] edge behavior for QueryPlanningTracker.topRulesByT…

…ime - followup patch ## What changes were proposed in this pull request? This is an addendum patch for SPARK-26129 that defines the edge case behavior for QueryPlanningTracker.topRulesByTime. ## How was this patch tested? Added unit tests for each behavior. Closes apache#23110 from rxin/SPARK-26129-1. Authored-by: Reynold Xin <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
japp2002 · Nov 22, 2018 · ab00533 · ab00533
1 parent 15c0384
commit ab00533
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 6 deletions.
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/QueryPlanningTracker.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/QueryPlanningTracker.scala
@@ -116,12 +116,19 @@ class QueryPlanningTracker {
 
   def phases: Map[String, Long] = phaseToTimeNs.asScala.toMap
 
-  /** Returns the top k most expensive rules (as measured by time). */
+  /**
+   * Returns the top k most expensive rules (as measured by time). If k is larger than the rules
+   * seen so far, return all the rules. If there is no rule seen so far or k <= 0, return empty seq.
+   */
   def topRulesByTime(k: Int): Seq[(String, RuleSummary)] = {
-    val orderingByTime: Ordering[(String, RuleSummary)] = Ordering.by(e => e._2.totalTimeNs)
-    val q = new BoundedPriorityQueue(k)(orderingByTime)
-    rulesMap.asScala.foreach(q.+=)
-    q.toSeq.sortBy(r => -r._2.totalTimeNs)
+    if (k <= 0) {
+      Seq.empty
+    } else {
+      val orderingByTime: Ordering[(String, RuleSummary)] = Ordering.by(e => e._2.totalTimeNs)
+      val q = new BoundedPriorityQueue(k)(orderingByTime)
+      rulesMap.asScala.foreach(q.+=)
+      q.toSeq.sortBy(r => -r._2.totalTimeNs)
+    }
   }
 
 }
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/QueryPlanningTrackerSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/QueryPlanningTrackerSuite.scala
@@ -62,17 +62,24 @@ class QueryPlanningTrackerSuite extends SparkFunSuite {
 
   test("topRulesByTime") {
     val t = new QueryPlanningTracker
+
+    // Return empty seq when k = 0
+    assert(t.topRulesByTime(0) == Seq.empty)
+    assert(t.topRulesByTime(1) == Seq.empty)
+
     t.recordRuleInvocation("r2", 2, effective = true)
     t.recordRuleInvocation("r4", 4, effective = true)
     t.recordRuleInvocation("r1", 1, effective = false)
     t.recordRuleInvocation("r3", 3, effective = false)
 
+    // k <= total size
+    assert(t.topRulesByTime(0) == Seq.empty)
     val top = t.topRulesByTime(2)
     assert(top.size == 2)
     assert(top(0)._1 == "r4")
     assert(top(1)._1 == "r3")
 
-    // Don't crash when k > total size
+    // k > total size
     assert(t.topRulesByTime(10).size == 4)
   }
 }