[SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest #6851

adrian-wang · 2015-06-17T07:28:36Z

rxin · 2015-06-17T07:30:15Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/ConditionalExpressionSuite.scala

@@ -134,4 +134,21 @@ class ConditionalExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
    checkEvaluation(CaseKeyWhen(literalNull, Seq(c2, c5, c1, c6)), "c", row)
  }

+  test("greatest/least") {
+    val row = create_row(1, 2, "a", "b", "c")


we need to test this for all data types.

chenghao-intel · 2015-06-17T07:42:06Z

Don't forget to implement the def foldable.

SparkQA · 2015-06-17T09:23:03Z

Test build #35027 has finished for PR 6851 at commit 3e25c6c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Expression*)
- case class Greatest(children: Expression*)

SparkQA · 2015-07-09T12:38:14Z

Test build #36926 has finished for PR 6851 at commit e478369.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Expression*) extends Expression
- case class Greatest(children: Expression*) extends Expression

chenghao-intel · 2015-07-10T02:24:54Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

+    greatest((columnName +: columnNames).map(Column.apply): _*)
+
+  /**
+    * Computes hex value of the given column


Nit:
/**

Computes hex value for the given column.
*

chenghao-intel · 2015-07-10T02:25:48Z

Can you rebase the code?

SparkQA · 2015-07-10T03:03:39Z

Test build #36999 has finished for PR 6851 at commit c1f6824.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-07-10T04:47:21Z

Test build #37001 has finished for PR 6851 at commit 7a6bdbb.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Expression*) extends Expression
- case class Greatest(children: Expression*) extends Expression

adrian-wang · 2015-07-10T05:05:20Z

retest this please.

SparkQA · 2015-07-10T05:08:34Z

Test build #37011 has finished for PR 6851 at commit 7a6bdbb.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

adrian-wang · 2015-07-10T05:19:34Z

retest this please.

SparkQA · 2015-07-10T07:04:12Z

Test build #37012 has finished for PR 6851 at commit 7a6bdbb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Expression*) extends Expression
- case class Greatest(children: Expression*) extends Expression

adrian-wang · 2015-07-10T07:31:18Z

@rxin @davies

davies · 2015-07-12T01:38:39Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

   * @since 1.5.0
   */
+  @scala.annotation.varargs
+  def greatest(exprs: Column*): Column = Greatest(exprs.map(_.expr): _*)


Should we check that exprs have at least two columns? Also for others, it's good to fail fast.

SparkQA · 2015-07-13T04:38:15Z

Test build #37117 has finished for PR 6851 at commit 0f1bff2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Expression*) extends Expression
- case class Greatest(children: Expression*) extends Expression

adrian-wang · 2015-07-13T06:55:27Z

cc @davies

davies · 2015-07-13T07:14:12Z

LGTM, merging this into master, thanks!

rxin · 2015-07-13T07:56:25Z

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

@@ -312,3 +313,103 @@ case class CaseKeyWhen(key: Expression, branches: Seq[Expression]) extends CaseW
    }.mkString
  }
 }
+
+case class Least(children: Expression*) extends Expression {


note: this should be Seq[Expression], not vararg

chenghao-intel zhichao-li qiansl127 Author: Daoyuan Wang <daoyuan.wang@intel.com> Closes apache#6851 from adrian-wang/udflg and squashes the following commits: 0f1bff2 [Daoyuan Wang] address comments from davis 7a6bdbb [Daoyuan Wang] add '.' for hex() c1f6824 [Daoyuan Wang] add codegen, test for all types ec625b0 [Daoyuan Wang] conditional function: least/greatest

adrian-wang · 2015-07-13T10:23:21Z

oh, it seems this pr has been merged but stay unclosed...
@davies @rxin

srowen · 2015-07-13T10:39:03Z

@adrian-wang probably a sync problem. It was evidently merged into master. You can just close it too.

SparkQA · 2015-07-13T10:54:51Z

Test build #37132 has finished for PR 6851 at commit 22e8f3d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Least(children: Seq[Expression]) extends Expression
- case class Greatest(children: Seq[Expression]) extends Expression

rxin · 2015-07-13T18:40:47Z

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

@@ -312,3 +313,103 @@ case class CaseKeyWhen(key: Expression, branches: Seq[Expression]) extends CaseW
    }.mkString
  }
 }
+
+case class Least(children: Seq[Expression]) extends Expression {


can you add scaladoc explaining the semantics of this expression? also do it for greatest.

rxin · 2015-07-13T18:42:29Z

@adrian-wang I found that happening quite a bit lately. Can you close the pr and just submit followup patches to address my comments? thanks.

This is a follow up of remaining comments from #6851 Author: Daoyuan Wang <daoyuan.wang@intel.com> Closes #7387 from adrian-wang/udflgfollow and squashes the following commits: 6163e62 [Daoyuan Wang] add skipping null values e8c2e09 [Daoyuan Wang] use seq 8362966 [Daoyuan Wang] pr6851 follow up

yjshen · 2015-07-29T14:57:21Z

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

+  private lazy val ordering = TypeUtils.getOrdering(dataType)
+
+  override def checkInputDataTypes(): TypeCheckResult = {
+    if (children.map(_.dataType).distinct.count(_ != NullType) > 1) {


Why count against not NullType? any reason here?

i think the problem is that null literals are not converted into any non-null type when it happens in greatest.

rxin reviewed Jun 17, 2015
View reviewed changes

adrian-wang force-pushed the udflg branch from 3e25c6c to e478369 Compare July 9, 2015 10:55

chenghao-intel reviewed Jul 10, 2015
View reviewed changes

adrian-wang added 2 commits July 9, 2015 20:00

conditional function: least/greatest

ec625b0

add codegen, test for all types

c1f6824

adrian-wang force-pushed the udflg branch from e478369 to c1f6824 Compare July 10, 2015 03:00

adrian-wang changed the title ~~[SPARK-8203] [SPARK-8204] [WIP] [SQL] conditional function: least/greatest~~ [SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest Jul 10, 2015

add '.' for hex()

7a6bdbb

davies reviewed Jul 12, 2015
View reviewed changes

address comments from davis

0f1bff2

rxin reviewed Jul 13, 2015
View reviewed changes

adrian-wang added 2 commits July 13, 2015 16:03

use seq for case class

86fb049

use seq for case class in functions

22e8f3d

rxin reviewed Jul 13, 2015
View reviewed changes

adrian-wang closed this Jul 14, 2015

adrian-wang mentioned this pull request Jul 14, 2015

[SPARK-6851] [SQL] function least/greatest follow up #7387

Closed

yjshen reviewed Jul 29, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest #6851

[SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest #6851

adrian-wang commented Jun 17, 2015

rxin Jun 17, 2015

chenghao-intel commented Jun 17, 2015

SparkQA commented Jun 17, 2015

SparkQA commented Jul 9, 2015

chenghao-intel Jul 10, 2015

chenghao-intel commented Jul 10, 2015

SparkQA commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

davies Jul 12, 2015

SparkQA commented Jul 13, 2015

adrian-wang commented Jul 13, 2015

davies commented Jul 13, 2015

rxin Jul 13, 2015

adrian-wang commented Jul 13, 2015

srowen commented Jul 13, 2015

SparkQA commented Jul 13, 2015

rxin Jul 13, 2015

rxin commented Jul 13, 2015

yjshen Jul 29, 2015

rxin Jul 30, 2015

[SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest #6851

[SPARK-8203] [SPARK-8204] [SQL] conditional function: least/greatest #6851

Conversation

adrian-wang commented Jun 17, 2015

rxin Jun 17, 2015

Choose a reason for hiding this comment

chenghao-intel commented Jun 17, 2015

SparkQA commented Jun 17, 2015

SparkQA commented Jul 9, 2015

chenghao-intel Jul 10, 2015

Choose a reason for hiding this comment

chenghao-intel commented Jul 10, 2015

SparkQA commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

SparkQA commented Jul 10, 2015

adrian-wang commented Jul 10, 2015

davies Jul 12, 2015

Choose a reason for hiding this comment

SparkQA commented Jul 13, 2015

adrian-wang commented Jul 13, 2015

davies commented Jul 13, 2015

rxin Jul 13, 2015

Choose a reason for hiding this comment

adrian-wang commented Jul 13, 2015

srowen commented Jul 13, 2015

SparkQA commented Jul 13, 2015

rxin Jul 13, 2015

Choose a reason for hiding this comment

rxin commented Jul 13, 2015

yjshen Jul 29, 2015

Choose a reason for hiding this comment

rxin Jul 30, 2015

Choose a reason for hiding this comment