[SPARK-23940][SQL] Add transform_values SQL function #22045

codeatri · 2018-08-08T19:35:57Z

What changes were proposed in this pull request?

This pr adds transform_values function which applies the function to each entry of the map and transforms the values.

> SELECT transform_values(map(array(1, 2, 3), array(1, 2, 3)), (k,v) -> v + 1);
       map(1->2, 2->3, 3->4)

> SELECT transform_values(map(array(1, 2, 3), array(1, 2, 3)), (k,v) -> k + v);
       map(1->2, 2->4, 3->6)

How was this patch tested?

New Tests added to
DataFrameFunctionsSuite
HigherOrderFunctionsSuite
SQLQueryTestSuite

mn-mikke · 2018-08-08T21:46:29Z

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

+
+/**
+ * Transform Values for every entry of the map by applying transform_values function.
+ * Returns map wth transformed values


typos: Transforms values; with
Maybe can you think of a better comment?

mn-mikke · 2018-08-08T21:51:42Z

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

+usage = "_FUNC_(expr, func) - Transforms values in the map using the function.",
+examples = """
+    Examples:
+       > SELECT _FUNC_(map(array(1, 2, 3), array(1, 2, 3), (k,v) -> k + 1);


nit:(k, v) and maybe I would use v + 1 instead of k + 1.

mn-mikke · 2018-08-08T21:56:41Z

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

+
+  override def dataType: DataType = {
+    val map = input.dataType.asInstanceOf[MapType]
+    MapType(map.keyType, function.dataType, map.valueContainsNull)


map.valueContainsNull -> function.nullable?

mn-mikke · 2018-08-08T22:01:54Z

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

+    MapType(map.keyType, function.dataType, map.valueContainsNull)
+  }
+
+  override def inputTypes: Seq[AbstractDataType] = Seq(MapType, expectingFunctionType)


This is already specified by MapBasedSimpleHigherOrderFunction.

mn-mikke · 2018-08-08T22:07:01Z

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

+  @transient val (keyType, valueType, valueContainsNull) =
+    HigherOrderFunction.mapKeyValueArgumentType(input.dataType)
+
+  override def bind(f: (Expression, Seq[(DataType, Boolean)]) => LambdaFunction):


nit: formatting

ueshin · 2018-08-15T03:47:54Z

ok to test.

SparkQA · 2018-08-15T03:55:14Z

Test build #94783 has finished for PR 22045 at commit b73106d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin

Thanks for working on both transform_keys and transform_values!
The comments here are almost the same as I commented for transform_keys.
Maybe we can follow the changes there after it is merged.

ueshin · 2018-08-15T03:49:23Z

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

+usage = "_FUNC_(expr, func) - Transforms values in the map using the function.",
+examples = """
+    Examples:
+       > SELECT _FUNC_(map(array(1, 2, 3), array(1, 2, 3), (k, v) -> v + 1);


nit: we need one more right parenthesis after the second array(1, 2, 3)?

@ueshin Thanks for the review! and yes I agree, I made the same mistakes in both the PR's. I was waiting for the transform_key to converge so that I can make the same changes here.

ueshin · 2018-08-15T03:50:15Z

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

+    MapType(map.keyType, function.dataType, function.nullable)
+  }
+
+  @transient val MapType(keyType, valueType, valueContainsNull) = argument.dataType


lazy val?
Could you add a test when argument is not a map in invalid cases of DataFrameFunctionsSuite?

ueshin · 2018-08-15T03:50:41Z

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

+
+  override def dataType: DataType = {
+    val map = argument.dataType.asInstanceOf[MapType]
+    MapType(map.keyType, function.dataType, function.nullable)


We can use keyType from the following val?

ueshin · 2018-08-15T03:52:17Z

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

+    val LambdaFunction(
+    _, (keyVar: NamedLambdaVariable) :: (valueVar: NamedLambdaVariable) :: Nil, _) = function
+    (keyVar, valueVar)
+  }


nit: how about:

@transient lazy val LambdaFunction(_, (keyVar: NamedLambdaVariable) :: (valueVar: NamedLambdaVariable) :: Nil, _) = function

ueshin · 2018-08-15T03:53:11Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HigherOrderFunctionsSuite.scala

+  def transformValues(expr: Expression, f: (Expression, Expression) => Expression): Expression = {
+    val valueType = expr.dataType.asInstanceOf[MapType].valueType
+    val keyType = expr.dataType.asInstanceOf[MapType].keyType
+    TransformValues(expr, createLambda(keyType, false, valueType, true, f))


We should use valueContainsNull instead of true?

ueshin · 2018-08-15T03:53:34Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HigherOrderFunctionsSuite.scala

+  test("TransformValues") {
+    val ai0 = Literal.create(
+      Map(1 -> 1, 2 -> 2, 3 -> 3),
+      MapType(IntegerType, IntegerType))


Can you add valueContainsNull explicitly?

ueshin · 2018-08-15T03:55:12Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/HigherOrderFunctionsSuite.scala

+      MapType(IntegerType, IntegerType))
+    val ain = Literal.create(
+      Map.empty[Int, Int],
+      MapType(IntegerType, IntegerType))


Can you add tests for Literal.create(null, MapType(IntegerType, IntegerType))?

ueshin · 2018-08-15T03:56:15Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

@@ -2302,6 +2302,210 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    assert(ex5.getMessage.contains("function map_zip_with does not support ordering on type map"))
  }

+  test("transform values function - test various primitive data types combinations") {


We don't need so many cases here. We only need to verify the api works end to end.
Evaluation checks of the function should be in HigherOrderFunctionsSuite.

ueshin · 2018-08-15T03:57:15Z

sql/core/src/test/resources/sql-tests/inputs/higher-order-functions.sql

+
+create or replace temporary view nested as values
+  (1, map(1,1,2,2,3,3)),
+  (2, map(4,4,5,5,6,6))


nit:

(1, map(1, 1, 2, 2, 3, 3)), (2, map(4, 4, 5, 5, 6, 6))

SparkQA · 2018-08-15T23:43:01Z

Test build #94827 has finished for PR 22045 at commit daf7935.

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

ueshin

LGTM except for some nits.

ueshin · 2018-08-16T03:40:55Z

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

+ * Returns a map that applies the function to each value of the map.
+ */
+@ExpressionDescription(
+usage = "_FUNC_(expr, func) - Transforms values in the map using the function.",


nit: indent

ueshin · 2018-08-16T03:41:05Z

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

+ */
+@ExpressionDescription(
+usage = "_FUNC_(expr, func) - Transforms values in the map using the function.",
+examples = """


ueshin · 2018-08-16T03:41:13Z

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

+       > SELECT _FUNC_(map(array(1, 2, 3), array(1, 2, 3)), (k, v) -> k + v);
+        map(array(1, 2, 3), array(2, 4, 6))
+  """,
+since = "2.4.0")


ueshin · 2018-08-16T03:51:26Z

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

+  }
+
+  @transient lazy val LambdaFunction(
+  _, (keyVar: NamedLambdaVariable) :: (valueVar: NamedLambdaVariable) :: Nil, _) = function


nit: indent

ueshin · 2018-08-16T03:56:14Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

+    def testInvalidLambdaFunctions(): Unit = {
+
+      val ex1 = intercept[AnalysisException] {
+        dfExample1.selectExpr("transform_values(i, k -> k )")


nit: remove an extra space after k -> k.

ueshin · 2018-08-16T08:11:42Z

@codeatri Could you fix the conflicts please? Thanks!

SparkQA · 2018-08-16T10:38:17Z

Test build #94843 has finished for PR 22045 at commit 56d08ef.

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

mn-mikke · 2018-08-16T11:27:24Z

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

+
+  @transient lazy val MapType(keyType, valueType, valueContainsNull) = argument.dataType
+
+  override def dataType: DataType = MapType(keyType, function.dataType, valueContainsNull)


Shouldn't the dataType be defined as MapType(keyType, function.dataType, function.nullable)?

SparkQA · 2018-08-17T00:00:26Z

Test build #94864 has finished for PR 22045 at commit 3382e1a.

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

ueshin · 2018-08-17T02:49:32Z

Thanks! merging to master.

## What changes were proposed in this pull request? - Revert [SPARK-23935][SQL] Adding map_entries function: #21236 - Revert [SPARK-23937][SQL] Add map_filter SQL function: #21986 - Revert [SPARK-23940][SQL] Add transform_values SQL function: #22045 - Revert [SPARK-23939][SQL] Add transform_keys function: #22013 - Revert [SPARK-23938][SQL] Add map_zip_with function: #22017 - Revert the changes of map_entries in [SPARK-24331][SPARKR][SQL] Adding arrays_overlap, array_repeat, map_entries to SparkR: #21434 ## How was this patch tested? The existing tests. Closes #22827 from gatorsmile/revertMap2.4. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Added transform_values function

68392e3

codeatri changed the title ~~[SPARK-23939][SQL] Add transform_values SQL function~~ [SPARK-23940][SQL] Add transform_values SQL function Aug 8, 2018

added test improvements

fd05645

mn-mikke reviewed Aug 8, 2018

View reviewed changes

codeatri added 3 commits August 8, 2018 16:07

addressed review comments

cdecd32

Merge branch 'master' into SPARK-23940

7df3ac5

Merge master Refactoring changes

b73106d

ueshin reviewed Aug 15, 2018

View reviewed changes

review comments

daf7935

ueshin reviewed Aug 16, 2018

View reviewed changes

post review

56d08ef

mn-mikke reviewed Aug 16, 2018

View reviewed changes

codeatri added 2 commits August 16, 2018 12:58

Merging Develop and resolving conflicts

9e89565

adding new line

3382e1a

asfgit closed this in f161409 Aug 17, 2018

gatorsmile mentioned this pull request Oct 25, 2018

[SPARK-25832][SQL][BRANCH-2.4] Revert newly added map related functions #22827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23940][SQL] Add transform_values SQL function #22045

[SPARK-23940][SQL] Add transform_values SQL function #22045

codeatri commented Aug 8, 2018 •

edited

Loading

mn-mikke Aug 8, 2018

mn-mikke Aug 8, 2018

mn-mikke Aug 8, 2018

mn-mikke Aug 8, 2018

mn-mikke Aug 8, 2018

ueshin commented Aug 15, 2018

SparkQA commented Aug 15, 2018

ueshin left a comment

ueshin Aug 15, 2018

codeatri Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

ueshin Aug 15, 2018

SparkQA commented Aug 15, 2018

ueshin left a comment

ueshin Aug 16, 2018

ueshin Aug 16, 2018

ueshin Aug 16, 2018

ueshin Aug 16, 2018

ueshin Aug 16, 2018

ueshin commented Aug 16, 2018

SparkQA commented Aug 16, 2018

mn-mikke Aug 16, 2018

SparkQA commented Aug 17, 2018

ueshin commented Aug 17, 2018


		@transient lazy val MapType(keyType, valueType, valueContainsNull) = argument.dataType

		override def dataType: DataType = MapType(keyType, function.dataType, valueContainsNull)

[SPARK-23940][SQL] Add transform_values SQL function #22045

[SPARK-23940][SQL] Add transform_values SQL function #22045

Conversation

codeatri commented Aug 8, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Aug 15, 2018

SparkQA commented Aug 15, 2018

ueshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 15, 2018

ueshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Aug 16, 2018

SparkQA commented Aug 16, 2018

Choose a reason for hiding this comment

SparkQA commented Aug 17, 2018

ueshin commented Aug 17, 2018

codeatri commented Aug 8, 2018 •

edited

Loading