You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/05/11 07:11:39 UTC

[GitHub] [spark] shahidki31 opened a new pull request #32498: [SPARK-35368][SQL]Update histogram statistics for RANGE operator stats estimation

shahidki31 opened a new pull request #32498:
URL: https://github.com/apache/spark/pull/32498


   
   ### What changes were proposed in this pull request?
   Update histogram statistics for RANGE operator stats estimation
   ### Why are the changes needed?
   If histogram optimization is enabled, this statistics can be used in various cost based optimizations.
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   Added UTs. Manual test.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631548474



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,38 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray.head
+    var lowerBinValue = getRangeValue(0)
+    percentileArray.tail.zipWithIndex.foreach { case (upperIndex, binId) =>
+      // Integer index for upper and lower values in the bin.
+      val upperIndexPos = math.ceil(upperIndex).toInt - 1
+      val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+      val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+      val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+      binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+
+      lowerBinValue = upperBinValue
+      lowerIndex = upperIndex
+    }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {

Review comment:
       ```
     private def getRangeValue(index: Int): Long = {
       assert(index >= 0, "index must be greater than and equal to 0")
       if (step == 0) {
         start + (numElements.toLong - index - 1) * step
       } else {
         start + index * step
       }
     }
   ```
   ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842634317


   **[Test build #138644 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138644/testReport)** for PR 32498 at commit [`44c5378`](https://github.com/apache/spark/commit/44c5378af777a3e54047a9636cdabf497751717d).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843612406


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43214/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839634303






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840281207


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43005/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633897836



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       Yes. Sure I will add a comment.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-837972977


   **[Test build #138362 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138362/testReport)** for PR 32498 at commit [`5191d9f`](https://github.com/apache/spark/commit/5191d9f8398abf5a49695eaa3ea1f2340623a596).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840018094


   **[Test build #138460 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138460/testReport)** for PR 32498 at commit [`ccd8f06`](https://github.com/apache/spark/commit/ccd8f068b604b085f086ee7a2861ddc1e2f5219c).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843585332






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840117966


   **[Test build #138466 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138466/testReport)** for PR 32498 at commit [`27ea38a`](https://github.com/apache/spark/commit/27ea38a96839dcf6749fe7ca86c0284d28a7eda0).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840190243


   **[Test build #138477 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138477/testReport)** for PR 32498 at commit [`0bb49b3`](https://github.com/apache/spark/commit/0bb49b3a15b4bf2c59916cce91d5aba285812079).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839958685


   **[Test build #138466 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138466/testReport)** for PR 32498 at commit [`27ea38a`](https://github.com/apache/spark/commit/27ea38a96839dcf6749fe7ca86c0284d28a7eda0).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631564557



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,38 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray.head
+    var lowerBinValue = getRangeValue(0)

Review comment:
       Yes, updated.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840724980


   **[Test build #138514 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138514/testReport)** for PR 32498 at commit [`6d54f7c`](https://github.com/apache/spark/commit/6d54f7c4c39dd61962f72771f09d480381039f29).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838375573


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138362/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839768168


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138445/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842336862


   Looks fine otherwise.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842661644


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43163/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839918693


   **[Test build #138462 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138462/testReport)** for PR 32498 at commit [`d314ba9`](https://github.com/apache/spark/commit/d314ba9fc16d33bb2a2a045693e29559c470a3f5).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840597570


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43034/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL]Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838050118


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42885/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631162814



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       I removed the condition. I have added the condition earlier because, if cbo is not enabled, other operators are not returning other statistics. But yes, in this case we have all the statistics readily available. Do we also need to remove histogram check as well?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(

Review comment:
       Done

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(
         distinctCount = Some(numElements),
         max = Some(maxVal),
         min = Some(minVal),
         nullCount = Some(0),
         avgLen = Some(LongType.defaultSize),
         maxLen = Some(LongType.defaultSize))
 
+      if (conf.histogramEnabled) {
+

Review comment:
       Removed

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(
         distinctCount = Some(numElements),
         max = Some(maxVal),
         min = Some(minVal),
         nullCount = Some(0),
         avgLen = Some(LongType.defaultSize),
         maxLen = Some(LongType.defaultSize))
 
+      if (conf.histogramEnabled) {
+
+        def getRangeValue(index: Int): Long = {

Review comment:
       Done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633579689



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       I'm probably missing something, but isn't this more like:
   ```
   (start + numElements.toLong - 1) + index * step
   ```
   This tries to step backwards from the end, right?
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       EDIT: Fixed my formula
   I'm probably missing something, but isn't this more like:
   ```
   start + (numElements.toLong + index - 1) * step
   ```
   This tries to step backwards from the end, right? `step` is already negative.
   Could also be maybe a little clearer as follows if I'm right.
   ```
   start + (numElements.toLong - 1) * step + index * step
   ```
   
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       EDIT: Fixed my formula
   I'm probably missing something, but isn't this more like:
   ```
   start + (numElements.toLong - 1) * (-step) + index * step
   ```
   This tries to step backwards from the end, right? `step` is negative.
   
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839867454


   **[Test build #138460 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138460/testReport)** for PR 32498 at commit [`ccd8f06`](https://github.com/apache/spark/commit/ccd8f068b604b085f086ee7a2861ddc1e2f5219c).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840083265


   **[Test build #138473 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138473/testReport)** for PR 32498 at commit [`fcf2705`](https://github.com/apache/spark/commit/fcf2705bab28eb9106a8f90b0f744acdf78c3356).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839321727


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138399/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840597570


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43034/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840037605


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138460/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631565143



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -283,14 +326,17 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
   private def checkStats(
       plan: LogicalPlan,
       expectedStatsCboOn: Statistics,
-      expectedStatsCboOff: Statistics): Unit = {
-    withSQLConf(SQLConf.CBO_ENABLED.key -> "true") {
+      expectedStatsCboOff: Statistics,
+      extraConfigs: Map[String, String] = Map.empty): Unit = {
+

Review comment:
       I am not sure I understand you here. Do we need to directly put the histogram configs inside this method? By default histogram is disabled and number of bins default value is 254.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840561675


   **[Test build #138514 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138514/testReport)** for PR 32498 at commit [`6d54f7c`](https://github.com/apache/spark/commit/6d54f7c4c39dd61962f72771f09d480381039f29).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839168422


   **[Test build #138387 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138387/testReport)** for PR 32498 at commit [`8280487`](https://github.com/apache/spark/commit/82804877e7148285d2232ffa5d3a3e1cf231e19e).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840337565






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839321727


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138399/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842634317


   **[Test build #138644 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138644/testReport)** for PR 32498 at commit [`44c5378`](https://github.com/apache/spark/commit/44c5378af777a3e54047a9636cdabf497751717d).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633855607



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       >This tries to step backwards from the end, right? step is negative
   
   Yes, we reverse the range values if the step is negative to compute histogram statistics.
   
   >I'm probably missing something, but isn't this more like:
       start + (numElements.toLong - 1) * (-step) + index * step
   
   I am not sure this gives the right range values if the step is negative. 
   For eg: start = 1 , step = -2, and numElements = 4, Range values  = [1, -1, -3, -5]
   But for computing histogram we need values like this [-5, -3, -1, 1]. So if the index = 0, it should return -5.
   
   With the formula in the PR => 1 + (4 - 0 -1 )* -2= -5
    With the formula you have given above  => 1 + (4 -1) *-(-2) + 0 = 7
   
   So I think the formula in the PR seems correct?
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839051298


   **[Test build #138399 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138399/testReport)** for PR 32498 at commit [`45845e3`](https://github.com/apache/spark/commit/45845e383291a635c7a0dd5113fb5a18679d35ee).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843560298


   **[Test build #138692 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138692/testReport)** for PR 32498 at commit [`7a364d8`](https://github.com/apache/spark/commit/7a364d80a5bce42ebf76f1a326ea21f8b851f795).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839768168


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138445/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r630844778



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       We need this?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(

Review comment:
       Please avoid using `var` where possible.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(
         distinctCount = Some(numElements),
         max = Some(maxVal),
         min = Some(minVal),
         nullCount = Some(0),
         avgLen = Some(LongType.defaultSize),
         maxLen = Some(LongType.defaultSize))
 
+      if (conf.histogramEnabled) {
+

Review comment:
       please remove this blank.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -767,14 +771,44 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
-      val colStat = ColumnStat(
+      var colStat = ColumnStat(
         distinctCount = Some(numElements),
         max = Some(maxVal),
         min = Some(minVal),
         nullCount = Some(0),
         avgLen = Some(LongType.defaultSize),
         maxLen = Some(LongType.defaultSize))
 
+      if (conf.histogramEnabled) {
+
+        def getRangeValue(index: Int): Long = {

Review comment:
       How about extracting this inner method as a private method?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843857290


   Thanks a lot @maropu , @srowen  for the reviews.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839180396


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138387/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839946313






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842759449


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138644/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838352006


   **[Test build #138362 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138362/testReport)** for PR 32498 at commit [`5191d9f`](https://github.com/apache/spark/commit/5191d9f8398abf5a49695eaa3ea1f2340623a596).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839180396


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138387/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840209168


   **[Test build #138473 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138473/testReport)** for PR 32498 at commit [`fcf2705`](https://github.com/apache/spark/commit/fcf2705bab28eb9106a8f90b0f744acdf78c3356).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843646341


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43214/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631536454



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    var binId = 0
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray(0)
+    var lowerBinValue = getRangeValue(0)
+    while (binId < numBins) {
+      val upperIndex = percentileArray(binId + 1)
+      val upperBinValue = getRangeValue(math.max(math.ceil(upperIndex).toInt - 1, 0))
+      val ndv = math.max(math.ceil(upperIndex).toInt - math.ceil(lowerIndex).toInt, 1)

Review comment:
       Yes, updated




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633543389



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)

Review comment:
       nit: how about rewriting it like this?
   ```
   
       val lowerIndexInitial = percentileArray.head
       val lowerBinValueInitial = getRangeValue(0)
       val (_, _, binArray) = percentileArray.tail
         .foldLeft((lowerIndexInitial, lowerBinValueInitial, Seq.empty[HistogramBin])) {
         case ((lowerIndex, lowerBinValue, binAr), upperIndex) =>
           // Integer index for upper and lower values in the bin.
           val upperIndexPos = math.ceil(upperIndex).toInt - 1
           val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
   
           val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
           val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
           // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
           (upperIndex, upperBinValue, binAr :+ HistogramBin(lowerBinValue, upperBinValue, ndv))
       }
       Histogram(height, binArray.toArray)
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839138562


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42921/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839990963






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840129440






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840292938


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138477/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631512825



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    var binId = 0

Review comment:
       This is not performance-intensive code, so could you use `zipWithIndex+map` instead of `while`?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    var binId = 0
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray(0)
+    var lowerBinValue = getRangeValue(0)
+    while (binId < numBins) {
+      val upperIndex = percentileArray(binId + 1)
+      val upperBinValue = getRangeValue(math.max(math.ceil(upperIndex).toInt - 1, 0))
+      val ndv = math.max(math.ceil(upperIndex).toInt - math.ceil(lowerIndex).toInt, 1)

Review comment:
       Please do not repeat `math.ceil(upperIndex)` here.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {

Review comment:
       `getHistogramStatistics` -> `computeHistogramStatistics()`

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -774,13 +774,17 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
+
+      val histogram = getHistogramStatistics

Review comment:
       I feel its better to wrap a return value of `computeHistogramStatistics ` with `Some` in the caller side because the method always return a value.
   https://github.com/apache/spark/pull/32498/files#diff-0ac2c89f8cc0d00e8fe7717b01697f36f20fe8abf2def09bcfde0ad84b30e467R814

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -774,13 +774,17 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
+
+      val histogram = getHistogramStatistics

Review comment:
       Let's check `conf.histogramEnabled` here;
   ```
   val histogram = if (conf.histogramEnabled) {
     Some(computeHistogramStatistics())
   } else {
     None
   }
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839573760


   **[Test build #138445 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138445/testReport)** for PR 32498 at commit [`89457c8`](https://github.com/apache/spark/commit/89457c8fec3ef2ecae6e9b752b0c7322ee54b7e9).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840037605


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138460/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843646330


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43214/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843586584


   **[Test build #138693 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138693/testReport)** for PR 32498 at commit [`bb69f84`](https://github.com/apache/spark/commit/bb69f84a3d05acec239db571afbfe2f41007d9ce).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842750526


   **[Test build #138644 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138644/testReport)** for PR 32498 at commit [`44c5378`](https://github.com/apache/spark/commit/44c5378af777a3e54047a9636cdabf497751717d).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838797638


   **[Test build #138387 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138387/testReport)** for PR 32498 at commit [`8280487`](https://github.com/apache/spark/commit/82804877e7148285d2232ffa5d3a3e1cf231e19e).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838797638


   **[Test build #138387 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138387/testReport)** for PR 32498 at commit [`8280487`](https://github.com/apache/spark/commit/82804877e7148285d2232ffa5d3a3e1cf231e19e).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633579689



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       I'm probably missing something, but isn't this more like:
   ```
   (start + numElements.toLong - 1) + index * step
   ```
   This tries to step backwards from the end, right?
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839297647


   **[Test build #138399 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138399/testReport)** for PR 32498 at commit [`45845e3`](https://github.com/apache/spark/commit/45845e383291a635c7a0dd5113fb5a18679d35ee).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633876565



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       OK, I thought it might mean the desired result was [7, 5, 3, 1] in this case. You step back from the start. But it has to be reversed? OK if it needs to be [-5, -3, -1, 1] yeah that looks right. A comment to this effect might be helpful.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633579689



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       EDIT: Fixed my formula
   I'm probably missing something, but isn't this more like:
   ```
   start + (numElements.toLong - 1) * (-step) + index * step
   ```
   This tries to step backwards from the end, right? `step` is negative.
   
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633897836



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       Yes. I added a comment.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839996564


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42987/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839918693


   **[Test build #138462 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138462/testReport)** for PR 32498 at commit [`d314ba9`](https://github.com/apache/spark/commit/d314ba9fc16d33bb2a2a045693e29559c470a3f5).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633858095



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)

Review comment:
       Updated. Thanks




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843646341


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43214/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839657500


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42966/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840404804






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838375573


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138362/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840106070


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138462/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631564790



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -77,12 +92,21 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(4),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step") {
     val range = Range(-10, -20, -2, None)
+    val histogramBins = new Array[HistogramBin](3)
+    histogramBins(0) = HistogramBin(-18.0, -16.0, 2)
+    histogramBins(1) = HistogramBin(-16.0, -12.0, 2)
+    histogramBins(2) = HistogramBin(-12.0, -10.0, 1)

Review comment:
       Added assert to check if `range.numElements` and `ndv` are same

##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -97,12 +121,24 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(-10),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step where end minus start not divisible by step") {
+
     val range = Range(-10, -20, -3, None)
+
+    val histogramBins = new Array[HistogramBin](3)
+    histogramBins(0) = HistogramBin(-19.0, -16.0, 2)
+    histogramBins(1) = HistogramBin(-16.0, -13.0, 1)
+    histogramBins(2) = HistogramBin(-13.0, -10.0, 1)

Review comment:
       Updated




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633543389



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)

Review comment:
       nit: how about rewriting it like this?
   ```
   
       val lowerIndexInitial = percentileArray.head
       val lowerBinValueInitial = getRangeValue(0)
       val (_, _, binArray) = percentileArray.tail
         .foldLeft((lowerIndexInitial, lowerBinValueInitial, Seq.empty[HistogramBin])) {
         case ((lowerIndex, lowerBinValue, binAr), upperIndex) =>
           // Integer index for upper and lower values in the bin.
           val upperIndexPos = math.ceil(upperIndex).toInt - 1
           val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
   
           val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
           val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
           // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
           (upperIndex, upperBinValue, binAr :+ HistogramBin(lowerBinValue, upperBinValue, ndv))
       }
       Histogram(height, binArray.toArray)
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840387634


   **[Test build #138494 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138494/testReport)** for PR 32498 at commit [`b7a6cc7`](https://github.com/apache/spark/commit/b7a6cc71110fe8de45e8c74d487ebd23b7942f34).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839915832


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42981/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842661644


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43163/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633857010



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       >Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be start always in that case anyway, so step < 0 as a condition?
   
   Yes, if step=0 both the conditions will return same start element. I have updated the condition to step < 0.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631551951



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -77,12 +92,21 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(4),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step") {
     val range = Range(-10, -20, -2, None)
+    val histogramBins = new Array[HistogramBin](3)
+    histogramBins(0) = HistogramBin(-18.0, -16.0, 2)
+    histogramBins(1) = HistogramBin(-16.0, -12.0, 2)
+    histogramBins(2) = HistogramBin(-12.0, -10.0, 1)

Review comment:
       nit:
   ```
       val histogramBins = Array(
         HistogramBin(-18.0, -16.0, 2),
         HistogramBin(-16.0, -12.0, 2),
         HistogramBin(-12.0, -10.0, 1))
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840190243


   **[Test build #138477 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138477/testReport)** for PR 32498 at commit [`0bb49b3`](https://github.com/apache/spark/commit/0bb49b3a15b4bf2c59916cce91d5aba285812079).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r633579689



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,40 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    val lowerIndexInitial = percentileArray.head
+    val lowerBinValueInitial = getRangeValue(0)
+    percentileArray.tail.zipWithIndex
+      .foldLeft[(Double, Long)]((lowerIndexInitial, lowerBinValueInitial)) {
+        case ((lowerIndex, lowerBinValue), (upperIndex, binId)) =>
+          // Integer index for upper and lower values in the bin.
+          val upperIndexPos = math.ceil(upperIndex).toInt - 1
+          val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+          val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+          val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+          binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+          // Update the lowerIndex and lowerBinValue with upper ones for the next iteration.
+          (upperIndex, upperBinValue)
+      }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {
+    assert(index >= 0, "index must be greater than and equal to 0")
+    if (step <= 0) {
+      start + (numElements.toLong - index - 1) * step

Review comment:
       EDIT: Fixed my formula
   I'm probably missing something, but isn't this more like:
   ```
   start + (numElements.toLong + index - 1) * step
   ```
   This tries to step backwards from the end, right? `step` is already negative.
   Could also be maybe a little clearer as follows if I'm right.
   ```
   start + (numElements.toLong - 1) * step + index * step
   ```
   
   Also I assume step = 0 is an error actually, maybe it's caught earlier, but the result should be `start` always in that case anyway, so `step < 0` as a condition?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840292938


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138477/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838796881


   cc @wangyum @cloud-fan @HyukjinKwon  Kindly review


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843560298


   **[Test build #138692 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138692/testReport)** for PR 32498 at commit [`7a364d8`](https://github.com/apache/spark/commit/7a364d80a5bce42ebf76f1a326ea21f8b851f795).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840106070


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138462/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839657500


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42966/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840207182


   Kubernetes integration test unable to build dist.
   
   exiting with code: 1
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42998/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL]Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-837972977


   **[Test build #138362 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138362/testReport)** for PR 32498 at commit [`5191d9f`](https://github.com/apache/spark/commit/5191d9f8398abf5a49695eaa3ea1f2340623a596).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631536413



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -774,13 +774,17 @@ case class Range(
       } else {
         (start + (numElements - 1) * step, start)
       }
+
+      val histogram = getHistogramStatistics

Review comment:
       Done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839956385


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42983/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839751330


   **[Test build #138445 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138445/testReport)** for PR 32498 at commit [`89457c8`](https://github.com/apache/spark/commit/89457c8fec3ef2ecae6e9b752b0c7322ee54b7e9).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839138562


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42921/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839867454


   **[Test build #138460 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138460/testReport)** for PR 32498 at commit [`ccd8f06`](https://github.com/apache/spark/commit/ccd8f068b604b085f086ee7a2861ddc1e2f5219c).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840083265


   **[Test build #138473 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138473/testReport)** for PR 32498 at commit [`fcf2705`](https://github.com/apache/spark/commit/fcf2705bab28eb9106a8f90b0f744acdf78c3356).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840310425


   **[Test build #138494 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138494/testReport)** for PR 32498 at commit [`b7a6cc7`](https://github.com/apache/spark/commit/b7a6cc71110fe8de45e8c74d487ebd23b7942f34).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL]Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838050118


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42885/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840561675


   **[Test build #138514 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138514/testReport)** for PR 32498 at commit [`6d54f7c`](https://github.com/apache/spark/commit/6d54f7c4c39dd61962f72771f09d480381039f29).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631552022



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -97,12 +121,24 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(-10),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step where end minus start not divisible by step") {
+
     val range = Range(-10, -20, -3, None)
+
+    val histogramBins = new Array[HistogramBin](3)
+    histogramBins(0) = HistogramBin(-19.0, -16.0, 2)
+    histogramBins(1) = HistogramBin(-16.0, -13.0, 1)
+    histogramBins(2) = HistogramBin(-13.0, -10.0, 1)

Review comment:
       ditto




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840132990






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840132987






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843566696


   Kubernetes integration test unable to build dist.
   
   exiting with code: 1
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43213/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843841101


   Thank you, @shahidki31 . Merged to master.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839958685


   **[Test build #138466 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138466/testReport)** for PR 32498 at commit [`27ea38a`](https://github.com/apache/spark/commit/27ea38a96839dcf6749fe7ca86c0284d28a7eda0).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu closed pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu closed pull request #32498:
URL: https://github.com/apache/spark/pull/32498


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838856365


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42910/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843564494


   **[Test build #138692 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138692/testReport)** for PR 32498 at commit [`7a364d8`](https://github.com/apache/spark/commit/7a364d80a5bce42ebf76f1a326ea21f8b851f795).
    * This patch **fails to build**.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839051298


   **[Test build #138399 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138399/testReport)** for PR 32498 at commit [`45845e3`](https://github.com/apache/spark/commit/45845e383291a635c7a0dd5113fb5a18679d35ee).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843687426


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138693/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843585332






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL]Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838044831






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840264723


   **[Test build #138485 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138485/testReport)** for PR 32498 at commit [`1cf79bf`](https://github.com/apache/spark/commit/1cf79bf05bd66446629628aac99828bc776f012c).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840084693


   **[Test build #138462 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138462/testReport)** for PR 32498 at commit [`d314ba9`](https://github.com/apache/spark/commit/d314ba9fc16d33bb2a2a045693e29559c470a3f5).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839913239






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842759449


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138644/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840404804






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631536346



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    var binId = 0

Review comment:
       Yes, I updated this with `zipWithIndex`

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +793,36 @@ case class Range(
     }
   }
 
+  private def getHistogramStatistics = {

Review comment:
       Done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840746280


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138514/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840339405


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43014/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842659254


   Kubernetes integration test unable to build dist.
   
   exiting with code: 1
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43163/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631546439



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,38 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray.head
+    var lowerBinValue = getRangeValue(0)

Review comment:
       It looks we can remove `var` by using `foldLeft` instead of `foreach`. Could you?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631162814



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       I removed the condition as we have all the statistics available for the operator. I have added the condition earlier because, if cbo is not enabled, other operators are not returning other statistics. Do we also need to remove histogram check as well?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-842336862


   Looks fine otherwise.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631162814



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       I removed the condition. I have added the condition earlier because, if cbo is not enabled, other operators are not returning column statistics. But yes, in this case we have all the statistics readily available. Do we also need to remove histogram check as well?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840339405


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43014/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840746280


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138514/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843686748


   **[Test build #138693 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138693/testReport)** for PR 32498 at commit [`bb69f84`](https://github.com/apache/spark/commit/bb69f84a3d05acec239db571afbfe2f41007d9ce).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631551107



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -97,12 +121,24 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(-10),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step where end minus start not divisible by step") {
+

Review comment:
       nit: please revert this change.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631385071



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       I removed histogram config check as well. Because histogram information is readily available. Also refactored the code and UTs




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840283366


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43005/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839138522


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42921/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839132217


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42921/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838852137






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840286024


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43005/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839996564


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42987/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631566208



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -283,14 +326,17 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
   private def checkStats(
       plan: LogicalPlan,
       expectedStatsCboOn: Statistics,
-      expectedStatsCboOff: Statistics): Unit = {
-    withSQLConf(SQLConf.CBO_ENABLED.key -> "true") {
+      expectedStatsCboOff: Statistics,
+      extraConfigs: Map[String, String] = Map.empty): Unit = {
+

Review comment:
       Yes, removed the extra line




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840292283


   **[Test build #138477 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138477/testReport)** for PR 32498 at commit [`0bb49b3`](https://github.com/apache/spark/commit/0bb49b3a15b4bf2c59916cce91d5aba285812079).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631385071



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -759,6 +759,10 @@ case class Range(
   }
 
   override def computeStats(): Statistics = {
+    if (!conf.cboEnabled) {
+      return Statistics(sizeInBytes = LongType.defaultSize * numElements)
+    }
+

Review comment:
       I removed histogram config check as well. Because histogram information is readily available. Also refactored the code and Its




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631552121



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -283,14 +326,17 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
   private def checkStats(
       plan: LogicalPlan,
       expectedStatsCboOn: Statistics,
-      expectedStatsCboOff: Statistics): Unit = {
-    withSQLConf(SQLConf.CBO_ENABLED.key -> "true") {
+      expectedStatsCboOff: Statistics,
+      extraConfigs: Map[String, String] = Map.empty): Unit = {
+

Review comment:
       nit: remove this unnecessary change




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-838856365


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42910/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839573760


   **[Test build #138445 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138445/testReport)** for PR 32498 at commit [`89457c8`](https://github.com/apache/spark/commit/89457c8fec3ef2ecae6e9b752b0c7322ee54b7e9).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r634746436



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,39 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {

Review comment:
       > Trivial: you could put a return type here.Yes, I added the return type
   
   Yes, I updated with the return type.
   
   > Is there any concern with the overhead of computing the histogram? It only happens when enabled of course. I just wonder if we need to micro-optimize the loop below with something more performant if this is critical, but maybe it isn't.
   
   This is not a performance intensive path. This will execute during planning phase. Also the loop size is numBins, which is 254 by default. So I am not sure we need to micro-optimise the loop?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843687426


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/138693/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r634400887



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,39 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {

Review comment:
       Trivial: you could put a return type here.
   Is there any concern with the overhead of computing the histogram? It only happens when enabled of course. I just wonder if we need to micro-optimize the loop below with something more performant if this is critical, but maybe it isn't.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839956385


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42983/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840264723


   **[Test build #138485 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138485/testReport)** for PR 32498 at commit [`1cf79bf`](https://github.com/apache/spark/commit/1cf79bf05bd66446629628aac99828bc776f012c).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-839915832


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42981/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840388226


   **[Test build #138485 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138485/testReport)** for PR 32498 at commit [`1cf79bf`](https://github.com/apache/spark/commit/1cf79bf05bd66446629628aac99828bc776f012c).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] shahidki31 commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
shahidki31 commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631564612



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -789,6 +797,38 @@ case class Range(
     }
   }
 
+  private def computeHistogramStatistics() = {
+    val numBins = conf.histogramNumBins
+    val height = numElements.toDouble / numBins
+    val percentileArray = (0 to numBins).map(i => i * height).toArray
+
+    val binArray = new Array[HistogramBin](numBins)
+    var lowerIndex = percentileArray.head
+    var lowerBinValue = getRangeValue(0)
+    percentileArray.tail.zipWithIndex.foreach { case (upperIndex, binId) =>
+      // Integer index for upper and lower values in the bin.
+      val upperIndexPos = math.ceil(upperIndex).toInt - 1
+      val lowerIndexPos = math.ceil(lowerIndex).toInt - 1
+
+      val upperBinValue = getRangeValue(math.max(upperIndexPos, 0))
+      val ndv = math.max(upperIndexPos - lowerIndexPos, 1)
+      binArray(binId) = HistogramBin(lowerBinValue, upperBinValue, ndv)
+
+      lowerBinValue = upperBinValue
+      lowerIndex = upperIndex
+    }
+    Histogram(height, binArray)
+  }
+
+  // Utility method to compute histogram
+  private def getRangeValue(index: Int): Long = {

Review comment:
       Done

##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -97,12 +121,24 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(-10),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step where end minus start not divisible by step") {
+

Review comment:
       Done




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840591536






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] maropu commented on a change in pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32498:
URL: https://github.com/apache/spark/pull/32498#discussion_r631552581



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/statsEstimation/BasicStatsEstimationSuite.scala
##########
@@ -77,12 +92,21 @@ class BasicStatsEstimationSuite extends PlanTest with StatsEstimationTestBase {
                 max = Some(4),
                 nullCount = Some(0),
                 maxLen = Some(LongType.defaultSize),
-                avgLen = Some(LongType.defaultSize))))))
-    checkStats(range, expectedStatsCboOn = rangeStats, expectedStatsCboOff = rangeStats)
+                avgLen = Some(LongType.defaultSize),
+                histogram = histogram)))))
+    val extraConfig = Map(SQLConf.HISTOGRAM_ENABLED.key -> "true",
+      SQLConf.HISTOGRAM_NUM_BINS.key -> "3")
+    checkStats(range, expectedStatsCboOn = rangeStats,
+      expectedStatsCboOff = rangeStats, extraConfig)
   }
 
   test("range with negative step") {
     val range = Range(-10, -20, -2, None)
+    val histogramBins = new Array[HistogramBin](3)
+    histogramBins(0) = HistogramBin(-18.0, -16.0, 2)
+    histogramBins(1) = HistogramBin(-16.0, -12.0, 2)
+    histogramBins(2) = HistogramBin(-12.0, -10.0, 1)

Review comment:
       Could you check if `range.numElements` and the total `ndv` are the same?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA removed a comment on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840310425


   **[Test build #138494 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138494/testReport)** for PR 32498 at commit [`b7a6cc7`](https://github.com/apache/spark/commit/b7a6cc71110fe8de45e8c74d487ebd23b7942f34).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] SparkQA commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-843586584


   **[Test build #138693 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/138693/testReport)** for PR 32498 at commit [`bb69f84`](https://github.com/apache/spark/commit/bb69f84a3d05acec239db571afbfe2f41007d9ce).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] AmplabJenkins commented on pull request #32498: [SPARK-35368][SQL] Update histogram statistics for RANGE operator for stats estimation

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32498:
URL: https://github.com/apache/spark/pull/32498#issuecomment-840286024


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/43005/
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org