[SPARK-21117][SQL] Built-in SQL Function Support - WIDTH_BUCKET #18323

wangyum · 2017-06-16T06:30:59Z

What changes were proposed in this pull request?

Add build-in SQL function - WIDTH_BUCKET

Ref: https://docs.oracle.com/cd/B28359_01/olap.111/b28126/dml_functions_2137.htm#OLADM717

How was this patch tested?

unit tests and manual tests.

manual tests(compare spark-sql results with oracle):

select width_bucket(5.35, 0.024, 10.06, 5) from dual; -- 3 --

select width_bucket(3.14, 0, 4, 3) from dual;         -- 3 --
select width_bucket(2, 0, 4, 3) from dual;            -- 2 --
select width_bucket(-1, 0, 3.2, 4) from dual;         -- 0 -- 

select width_bucket(3.14, 4, 0, 3) from dual;         -- 1 --
select width_bucket(2, 4, 0, 3) from dual;            -- 2 --
select width_bucket(-1, 3.2, 0, 4) from dual;         -- 5 --

SparkQA · 2017-06-16T06:32:36Z

Test build #78160 has started for PR 18323 at commit 20fe567.

Conflicts: sql/core/src/test/resources/sql-tests/inputs/operators.sql sql/core/src/test/resources/sql-tests/results/operators.sql.out

SparkQA · 2017-06-16T06:42:34Z

Test build #78161 has started for PR 18323 at commit 3a0eaf7.

wangyum · 2017-06-16T12:24:16Z

Jenkins, retest this please

SparkQA · 2017-06-16T14:39:57Z

Test build #78181 has finished for PR 18323 at commit 3a0eaf7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Conflicts: sql/core/src/test/resources/sql-tests/inputs/operators.sql sql/core/src/test/resources/sql-tests/results/operators.sql.out

SparkQA · 2017-06-17T01:30:44Z

Test build #78196 has finished for PR 18323 at commit 3a5d46e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-06-19T02:21:51Z

cc @wzhfy

SparkQA · 2017-06-19T07:04:41Z

Test build #78245 has finished for PR 18323 at commit 7407541.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wzhfy · 2017-06-22T01:14:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

+    ev: ExprCode,
+    f: (String, String, String, String) => String): ExprCode = {
+    nullSafeCodeGen(ctx, ev, (eval1, eval2, eval3, eval4) => {
+      s"${ev.value} = ${f(eval1, eval2, eval3, eval3)};"


the last eval3 -> eval4

wzhfy · 2017-06-22T01:14:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

+  override def nullable: Boolean = children.exists(_.nullable)
+
+  /**
+   * Default behavior of evaluation according to the default nullability of TernaryExpression.


TernaryExpression -> QuaternaryExpression, please also fix other places

wzhfy · 2017-06-22T01:23:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

+   * If either of the sub-expressions is null, the result of this computation
+   * is assumed to be null.
+   *
+   * @param f function that accepts the 3 non-null evaluation result names of children


wzhfy · 2017-06-22T02:25:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

 }
+
+/**
+ *  The bucket number into which


nit: Returns the bucket number

wzhfy · 2017-06-22T02:27:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+   *  Returns the bucket number into which
+   *  the value of this expression would fall after being evaluated.
+   *
+   * @param expr id the expression for which the histogram is being created


nit: id -> is

wzhfy · 2017-06-22T02:44:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+   * @param numBucket is an An expression that resolves to
+   *                  a constant indicating the number of buckets
+   * @return Returns an long between 0 and numBucket+1 by mapping the expr into buckets defined by
+   *         the range [minValue, maxValue]


Both endpoints are included? Can you check with other databases and add a comment here?

Yes, test on Oracle:

width_Bucket(0, 1, 1, 1) -> 0, width_Bucket(20, 1, 1, 1) -> 2

wzhfy · 2017-06-22T02:45:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+    }
+
+    val result = if (minValue > maxValue) (numBucket - preResult) + 1 else preResult
+    result


nit: if (minValue > maxValue) (numBucket - preResult) + 1 else preResult

wzhfy · 2017-06-22T02:49:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+    val preResult: Long = if (expr < lower) {
+      0
+    } else if (expr >= upper) {
+      Math.addExact(numBucket, 1)


Do we really need to use addExact? In Oracle's doc, numBucket is an integer, then we can use numBucket + 1L here

wzhfy · 2017-06-22T02:52:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+    } else if (expr >= upper) {
+      Math.addExact(numBucket, 1)
+    } else {
+      (numBucket.toDouble * (expr - lower) / (upper - lower) + 1).toLong


what if upper == lower?

wzhfy · 2017-06-22T02:59:34Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/MathUtilsSuite.scala

+    assert(widthBucket(100, 100, 5000, 10) === 1)
+    assert(widthBucket(590, 100, 5000, 10) === 2)
+    assert(widthBucket(5000, 100, 5000, 10) === 11)
+    assert(widthBucket(6000, 100, 5000, 10) === 11)


Can you use the same cases from Oracle's doc? just to make sure we are getting the same results here.

Conflicts: sql/core/src/test/resources/sql-tests/results/operators.sql.out

SparkQA · 2017-06-23T10:14:53Z

Test build #78515 has finished for PR 18323 at commit 5c3ecee.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wzhfy · 2017-06-26T02:14:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+   *  Returns the bucket number into which
+   *  the value of this expression would fall after being evaluated.
+   *
+   * @param expr id the expression for which the histogram is being created


nit: id -> is

wzhfy · 2017-06-26T02:14:21Z

sql/core/src/test/resources/sql-tests/inputs/operators.sql

+
+-- width_bucket
+select width_bucket(5.35, 0.024, 10.06, 5);
+select width_bucket(5.35, 0.024, 10.06, -5);


add a case for wrong input type: select width_bucket(5.35, 0.024, 10.06, 0.5);

wzhfy · 2017-06-26T02:31:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+
+    if (numBucket <= 0) {
+      throw new AnalysisException(s"The num of bucket must be greater than 0, but got ${numBucket}")
+    }


Do we consider minValue == maxValue and numBucket > 1 valid input or not?
Please also add a test case for this.

If minValue == maxValue , then lower==upper, result is numBucket + 1L:

val lower: Double = Math.min(minValue, maxValue) val upper: Double = Math.max(minValue, maxValue) val result: Long = if (expr < lower) { 0 } else if (expr >= upper) { numBucket + 1L } else { (numBucket.toDouble * (expr - lower) / (upper - lower) + 1).toLong } if (minValue > maxValue) (numBucket - result) + 1 else result

I know, but my question is: should the possible result when minValue == maxValue only be 0, 1, or 2? e.g. should width_bucket(2, 1, 1, 100) = 101 or just throw an error due to invalid input?

wzhfy · 2017-06-26T02:34:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/MathUtils.scala

+   *                  a constant indicating the number of buckets
+   * @return Returns an long between 0 and numBucket+1 by mapping the expr into buckets defined by
+   *         the range [minValue, maxValue]. For example:
+   *         widthBucket(0, 1, 1, 1) -> 0, widthBucket(20, 1, 1, 1) -> 2.


Let's remove these examples in the description, they are just corner cases. My previous comment was just to make sure both ends should be included.

wzhfy · 2017-06-26T06:47:05Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/MathUtilsSuite.scala

+    assert(widthBucket(400, 100, 5000, 10) === 1)
+
+    // minValue == maxValue
+    assert(widthBucket(10, 4, 4, 15) === 16)


Is this behavior consistent with other databases? If so, then I'm fine with this and please ignore my previous comment.

Yes, This is Oracle's behavior:

SparkQA · 2017-06-26T07:04:58Z

Test build #78611 has finished for PR 18323 at commit 9e2cabb.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-06-26T09:08:15Z

retest this please.

SparkQA · 2017-06-26T11:08:37Z

Test build #78627 has finished for PR 18323 at commit 9e2cabb.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

wzhfy · 2017-06-27T01:41:35Z

This error appears in a few builds. Let's wait for the fix. Currently it looks good to me.
ping @gatorsmile

wzhfy · 2017-06-28T07:04:50Z

retest this please

SparkQA · 2017-06-28T09:28:24Z

Test build #78769 has finished for PR 18323 at commit 9e2cabb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-07-16T00:12:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+ *                 to the minimum end point of the acceptable range for expr
+ * @param maxValue is an expression that resolves
+ *                 to the maximum end point of the acceptable range for expr
+ * @param numBucket is an An expression that resolves to


an An -> an

gatorsmile · 2017-07-16T00:18:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+
+  override def nullSafeEval(ex: Any, min: Any, max: Any, num: Any): Any = {
+    MathUtils.widthBucket(
+      ex.asInstanceOf[Double],


What happened if the input is not a constant, but an foldable expression?

foldable expression is correct.

SparkQA · 2017-07-16T10:42:02Z

Test build #79641 has finished for PR 18323 at commit 507fcfb.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-07-16T18:38:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+ */
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+  usage = "_FUNC_(expr, min_value, max_value, num_bucket) - Returns an long between 0 and `num_buckets`+1 by mapping the `expr` into buckets defined by the range [`min_value`, `max_value`].",


Nit: an -> a

Returns the bucket to which operand would be assigned in an equidepth histogram with num_bucket buckets, in the range min_value to max_value.

SparkQA · 2017-08-01T09:24:44Z

Test build #80112 has finished for PR 18323 at commit e0478f5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala # sql/core/src/test/resources/sql-tests/inputs/operators.sql # sql/core/src/test/resources/sql-tests/results/operators.sql.out

SparkQA · 2017-09-16T16:32:46Z

Test build #81843 has finished for PR 18323 at commit 2e2b2ca.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rdehouss · 2018-07-09T20:11:24Z

Any news? What's blocking this PR to move on except the conflict to resolve since it's been 1 year?
This addition would be really great.

SparkQA · 2018-08-07T16:21:30Z

Test build #94376 has finished for PR 18323 at commit 2e2b2ca.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

rdehouss · 2018-11-10T15:09:01Z

Closed? Not merged? Will it come via another way?

gatorsmile · 2020-05-01T22:03:14Z

@wangyum Can we rebase it? We should add this support for compatibility.

SparkQA · 2020-05-01T22:08:10Z

Test build #122180 has finished for PR 18323 at commit 2e2b2ca.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala # sql/core/src/test/resources/sql-tests/results/operators.sql.out

SparkQA · 2020-05-03T06:45:46Z

Test build #122223 has finished for PR 18323 at commit 31ee943.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-05-03T10:47:10Z

You need to run SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *ExpressionsSchemaSuite" to update the golden file.

…hemaSuite"

SparkQA · 2020-05-03T16:27:40Z

Test build #122229 has finished for PR 18323 at commit 44ef7df.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-05-29T08:13:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+  expr: Expression,
+  minValue: Expression,
+  maxValue: Expression,
+  numBucket: Expression) extends Expression with ImplicitCastInputTypes {


Cloud we do WidthBucket (...) extends QuaternaryExpression with ImplicitCastInputTypes with CodegenFallback for simplicity? If we need the codegen support, I think its okay to do so in follow-up.

maropu · 2020-05-29T08:14:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+
+/**
+ *  Returns the bucket number into which
+ *  the value of this expression would fall after being evaluated.


nit: how about the format and the rephrasing below?

/** * Returns the bucket number into which the value of this expression would fall * after being evaluated. * * @param expr is the expression to compute a bucket number in the histogram * @param minValue is the minimum value of the histogram * @param maxValue is the maximum value of the histogram * @param numBucket is the number of buckets */

maropu · 2020-05-29T08:15:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+// scalastyle:off line.size.limit
+@ExpressionDescription(
+  usage = "_FUNC_(expr, min_value, max_value, num_bucket) - Returns the `bucket` to which operand would be assigned in an equidepth histogram with `num_bucket` buckets, in the range `min_value` to `max_value`.",
+  extended = """


extended -> examples. Also, plz add a since tag.

maropu · 2020-05-29T08:20:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+      } else if (evals(0) == null) {
+        null
+      } else {
+        MathUtils.widthBucket(


MathUtils.widthBucket is used only for this new expr now? If so, could we move the method to this expr?

maropu · 2020-05-29T08:26:50Z

Could you solve the conflict?

maropu · 2020-06-09T00:23:53Z

@wangyum If you don't time to update this, I could take this over. Thought?

wangyum · 2020-06-09T00:25:57Z

@maropu Please take it over. Thank you.

maropu · 2020-06-09T00:26:35Z

ok

### What changes were proposed in this pull request? This PR intends to add a build-in SQL function - `WIDTH_BUCKET`. It is the rework of #18323. Closes #18323 The other RDBMS references for `WIDTH_BUCKET`: - Oracle: https://docs.oracle.com/cd/B28359_01/olap.111/b28126/dml_functions_2137.htm#OLADM717 - PostgreSQL: https://www.postgresql.org/docs/current/functions-math.html - Snowflake: https://docs.snowflake.com/en/sql-reference/functions/width_bucket.html - Prestodb: https://prestodb.io/docs/current/functions/math.html - Teradata: https://docs.teradata.com/reader/kmuOwjp1zEYg98JsB8fu_A/Wa8vw69cGzoRyNULHZeudg - DB2: https://www.ibm.com/support/producthub/db2/docs/content/SSEPGG_11.5.0/com.ibm.db2.luw.sql.ref.doc/doc/r0061483.html?pos=2 ### Why are the changes needed? For better usability. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added unit tests. Closes #28764 from maropu/SPARK-21117. Lead-authored-by: Takeshi Yamamuro <[email protected]> Co-authored-by: Yuming Wang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Built-in SQL Function Support - WIDTH_BUCKET

20fe567

wangyum added 2 commits June 16, 2017 14:32

Merge remote-tracking branch 'origin/master' into width_bucket

cb682a9

Conflicts: sql/core/src/test/resources/sql-tests/inputs/operators.sql sql/core/src/test/resources/sql-tests/results/operators.sql.out

Merge master

3a0eaf7

Merge remote-tracking branch 'origin/master' into SPARK-21117

3a5d46e

Conflicts: sql/core/src/test/resources/sql-tests/inputs/operators.sql sql/core/src/test/resources/sql-tests/results/operators.sql.out

Merge branch 'master' into SPARK-21117

7407541

wzhfy reviewed Jun 22, 2017

View reviewed changes

wangyum added 2 commits June 23, 2017 13:26

Merge remote-tracking branch 'origin/master' into SPARK-21117

fda866f

Conflicts: sql/core/src/test/resources/sql-tests/results/operators.sql.out

Fix review comments

5c3ecee

wzhfy reviewed Jun 26, 2017

View reviewed changes

Add more test.

9e2cabb

wzhfy reviewed Jun 26, 2017

View reviewed changes

gatorsmile reviewed Jul 16, 2017

View reviewed changes

Fix typo.

507fcfb

gatorsmile reviewed Jul 16, 2017

View reviewed changes

Remove QuaternaryExpression

e0478f5

SayidJ approved these changes Sep 16, 2017

View reviewed changes

wangyum closed this Nov 10, 2018

gatorsmile reopened this May 1, 2020

wangyum added 2 commits May 3, 2020 10:59

Merge remote-tracking branch 'upstream/master' into SPARK-21117

ea5e4ae

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala # sql/core/src/test/resources/sql-tests/results/operators.sql.out

Merge upstream

31ee943

probot-autolabeler bot added the SQL label May 3, 2020

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *ExpressionsSc…

44ef7df

…hemaSuite"

maropu reviewed May 29, 2020

View reviewed changes

maropu mentioned this pull request Jun 9, 2020

[SPARK-21117][SQL] Built-in SQL Function Support - WIDTH_BUCKET #28764

Closed

wangyum closed this Jun 9, 2020

[SPARK-21117][SQL] Built-in SQL Function Support - WIDTH_BUCKET #18323

[SPARK-21117][SQL] Built-in SQL Function Support - WIDTH_BUCKET #18323

Uh oh!

Conversation

wangyum commented Jun 16, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jun 16, 2017

Uh oh!

SparkQA commented Jun 16, 2017

Uh oh!

wangyum commented Jun 16, 2017

Uh oh!

SparkQA commented Jun 16, 2017

Uh oh!

SparkQA commented Jun 17, 2017

Uh oh!

gatorsmile commented Jun 19, 2017

Uh oh!

SparkQA commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wzhfy Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 26, 2017

Uh oh!

wangyum commented Jun 26, 2017

Uh oh!

SparkQA commented Jun 26, 2017

Uh oh!

wzhfy commented Jun 27, 2017

Uh oh!

wzhfy commented Jun 28, 2017

Uh oh!

SparkQA commented Jun 28, 2017

Uh oh!

wzhfy Jun 26, 2017 •

edited

Loading

gatorsmile Jul 16, 2017 •

edited

Loading

maropu May 29, 2020 •

edited

Loading