[SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18418

viirya · 2017-06-26T04:32:35Z

What changes were proposed in this pull request?

The issue happens in ExternalMapToCatalyst. For example, the following codes create ExternalMapToCatalyst to convert Scala Map to catalyst map format.

val data = Seq.tabulate(10)(i => NestedData(1, Map("key" -> InnerData("name", i + 100))))
val ds = spark.createDataset(data)

The valueConverter in ExternalMapToCatalyst looks like:

if (isnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true))) null else named_struct(name, staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).name, true), value, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).value)

There is a CreateNamedStruct expression (named_struct) to create a row of InnerData.name and InnerData.value that are referred by ExternalMapToCatalyst_value52.

Because ExternalMapToCatalyst_value52 are local variable, when CreateNamedStruct splits expressions to individual functions, the local variable can't be accessed anymore.

How was this patch tested?

Jenkins tests.

…litExpressions.

SparkQA · 2017-06-26T06:06:52Z

Test build #78599 has finished for PR 18418 at commit c417e22.

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

viirya · 2017-06-26T06:19:45Z

retest this please.

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

Test build #78601 has finished for PR 18418 at commit bd0221a.

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

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

Test build #78609 has finished for PR 18418 at commit bd0221a.

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

viirya · 2017-06-26T07:06:25Z

retest this please.

SparkQA · 2017-06-26T08:40:38Z

Test build #78613 has finished for PR 18418 at commit bd0221a.

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

viirya · 2017-06-26T08:42:39Z

retest this please.

SparkQA · 2017-06-26T10:42:14Z

Test build #78625 has finished for PR 18418 at commit bd0221a.

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

viirya · 2017-06-26T13:15:20Z

retest this please.

SparkQA · 2017-06-26T15:29:45Z

Test build #78635 has finished for PR 18418 at commit bd0221a.

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

kiszk · 2017-06-26T18:17:04Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -736,7 +736,27 @@ class CodegenContext {
      // Cannot split these expressions because they are not created from a row object.
      return expressions.mkString("\n")
    }
-    splitExpressions(expressions, "apply", ("InternalRow", row) :: Nil)
+    splitExpressions(row, expressions, Seq.empty)


Do we need the check in lines 735-738? Now, we do the same check at lines 754-757, too.

Removed, thanks.

viirya · 2017-06-27T00:19:25Z

cc @cloud-fan

cloud-fan · 2017-06-27T02:02:26Z

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

+    // of the functions.
+    val (valExprCodes, valExprParams) = valExprs.map { expr =>
+      val exprCode = expr.genCode(ctx)
+      val lambdaVars = expr.collect {


this looks hacky, and why you only do it for CreateNamedStruct?

For example, CreateArray is another expression using splitExpressions. However, if the external type is an array, MapObjects will be used to construct the internal array. CreateMap is the same, I think.

IIRC there are a lot of places we use splitExpressions, shall we check all of them?

I'm not very sure about other places. I move the collecting of lambda variable to splitExpressions and let the possible places to do this check.

This approach doesn't work well after rethinking about this and more experiments.

A simpler approach is letting those lambda variables global as MapObjectsandCollectObjectsToMap` do.

SparkQA · 2017-06-27T02:07:12Z

Test build #78666 has finished for PR 18418 at commit f4cb190.

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

SparkQA · 2017-06-27T05:19:30Z

Test build #78678 has finished for PR 18418 at commit d6161d5.

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

cloud-fan · 2017-06-27T09:08:15Z

can you put more detailed information about what is the bug? I don't understand why it only happens for map inside case class.

cloud-fan

Yea I think this is the right fix according to how we deal with this kind of problem before. LGTM

cloud-fan · 2017-06-27T09:29:11Z

BTW please update PR tile to mention that it's a bug fix for ExternalMapToCatalyst

viirya · 2017-06-27T09:36:01Z

@cloud-fan Thanks. The PR title and description are both updated to reflect the change.

SparkQA · 2017-06-27T11:01:19Z

Test build #78690 has finished for PR 18418 at commit 65d8873.

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

SparkQA · 2017-06-27T11:46:13Z

Test build #78688 has finished for PR 18418 at commit eb061da.

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

cloud-fan · 2017-06-27T12:17:04Z

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

@@ -354,4 +357,9 @@ class DatasetPrimitiveSuite extends QueryTest with SharedSQLContext {
    checkDataset(Seq(PackageClass(1)).toDS(), PackageClass(1))
  }

+  test("SPARK-19104: lambda variables should work when parent expression splits generated codes") {


you should also update the test name...

miss it. updated.

SparkQA · 2017-06-27T14:35:23Z

Test build #78698 has finished for PR 18418 at commit cffb09b.

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

viirya · 2017-06-27T14:37:54Z

retest this please.

SparkQA · 2017-06-27T16:42:49Z

Test build #78701 has finished for PR 18418 at commit cffb09b.

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

cloud-fan · 2017-06-27T16:57:03Z

the test failure is unrelated, merging to master/2.2!

…e global The issue happens in `ExternalMapToCatalyst`. For example, the following codes create `ExternalMapToCatalyst` to convert Scala Map to catalyst map format. val data = Seq.tabulate(10)(i => NestedData(1, Map("key" -> InnerData("name", i + 100)))) val ds = spark.createDataset(data) The `valueConverter` in `ExternalMapToCatalyst` looks like: if (isnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true))) null else named_struct(name, staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).name, true), value, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).value) There is a `CreateNamedStruct` expression (`named_struct`) to create a row of `InnerData.name` and `InnerData.value` that are referred by `ExternalMapToCatalyst_value52`. Because `ExternalMapToCatalyst_value52` are local variable, when `CreateNamedStruct` splits expressions to individual functions, the local variable can't be accessed anymore. Jenkins tests. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes #18418 from viirya/SPARK-19104. (cherry picked from commit fd8c931) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…e global ## What changes were proposed in this pull request? The issue happens in `ExternalMapToCatalyst`. For example, the following codes create `ExternalMapToCatalyst` to convert Scala Map to catalyst map format. val data = Seq.tabulate(10)(i => NestedData(1, Map("key" -> InnerData("name", i + 100)))) val ds = spark.createDataset(data) The `valueConverter` in `ExternalMapToCatalyst` looks like: if (isnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true))) null else named_struct(name, staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).name, true), value, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).value) There is a `CreateNamedStruct` expression (`named_struct`) to create a row of `InnerData.name` and `InnerData.value` that are referred by `ExternalMapToCatalyst_value52`. Because `ExternalMapToCatalyst_value52` are local variable, when `CreateNamedStruct` splits expressions to individual functions, the local variable can't be accessed anymore. ## How was this patch tested? Jenkins tests. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#18418 from viirya/SPARK-19104.

…alyst should be global ## What changes were proposed in this pull request? This PR is backport of #18418 to Spark 2.1. [SPARK-21391](https://issues.apache.org/jira/browse/SPARK-21391) reported this problem in Spark 2.1. The issue happens in `ExternalMapToCatalyst`. For example, the following codes create ExternalMap`ExternalMapToCatalyst`ToCatalyst to convert Scala Map to catalyst map format. ``` val data = Seq.tabulate(10)(i => NestedData(1, Map("key" -> InnerData("name", i + 100)))) val ds = spark.createDataset(data) ``` The `valueConverter` in `ExternalMapToCatalyst` looks like: ``` if (isnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true))) null else named_struct(name, staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).name, true), value, assertnotnull(lambdavariable(ExternalMapToCatalyst_value52, ExternalMapToCatalyst_value_isNull52, ObjectType(class org.apache.spark.sql.InnerData), true)).value) ``` There is a `CreateNamedStruct` expression (`named_struct`) to create a row of `InnerData.name` and `InnerData.value` that are referred by `ExternalMapToCatalyst_value52`. Because `ExternalMapToCatalyst_value52` are local variable, when `CreateNamedStruct` splits expressions to individual functions, the local variable can't be accessed anymore. ## How was this patch tested? Added a new test suite into `DatasetPrimitiveSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #18627 from kiszk/SPARK-21391.

Add lambda variables into the parameters of functions generated by sp…

bd0221a

…litExpressions.

viirya force-pushed the SPARK-19104 branch from c417e22 to bd0221a Compare June 26, 2017 04:44

kiszk reviewed Jun 26, 2017

View reviewed changes

Address comment.

f4cb190

cloud-fan reviewed Jun 27, 2017

View reviewed changes

viirya force-pushed the SPARK-19104 branch 2 times, most recently from eb061da to 65d8873 Compare June 27, 2017 09:27

cloud-fan approved these changes Jun 27, 2017

View reviewed changes

viirya changed the title ~~[SPARK-19104][SQL] Lambda variables should work when parent expression splits generated codes~~ [SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global Jun 27, 2017

cloud-fan reviewed Jun 27, 2017

View reviewed changes

Simpler approach.

cffb09b

viirya force-pushed the SPARK-19104 branch from 65d8873 to cffb09b Compare June 27, 2017 12:53

asfgit closed this in fd8c931 Jun 27, 2017

This was referenced Jul 13, 2017

[BACKPORT-2.1][SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18626

Closed

[BACKPORT-2.1][SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18627

Closed

viirya deleted the SPARK-19104 branch December 27, 2023 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18418

[SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18418

viirya commented Jun 26, 2017 •

edited

Loading

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

kiszk Jun 26, 2017

viirya Jun 27, 2017

viirya commented Jun 27, 2017

cloud-fan Jun 27, 2017

viirya Jun 27, 2017

cloud-fan Jun 27, 2017

viirya Jun 27, 2017

viirya Jun 27, 2017

SparkQA commented Jun 27, 2017

SparkQA commented Jun 27, 2017

cloud-fan commented Jun 27, 2017

cloud-fan left a comment

cloud-fan commented Jun 27, 2017

viirya commented Jun 27, 2017

SparkQA commented Jun 27, 2017

SparkQA commented Jun 27, 2017

cloud-fan Jun 27, 2017

viirya Jun 27, 2017

SparkQA commented Jun 27, 2017

viirya commented Jun 27, 2017

SparkQA commented Jun 27, 2017

cloud-fan commented Jun 27, 2017

[SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18418

[SPARK-19104][SQL] Lambda variables in ExternalMapToCatalyst should be global #18418

Conversation

viirya commented Jun 26, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

viirya commented Jun 26, 2017

SparkQA commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Jun 27, 2017

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 Jun 27, 2017

SparkQA commented Jun 27, 2017

cloud-fan commented Jun 27, 2017

cloud-fan left a comment

Choose a reason for hiding this comment

cloud-fan commented Jun 27, 2017

viirya commented Jun 27, 2017

SparkQA commented Jun 27, 2017

SparkQA commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 27, 2017

viirya commented Jun 27, 2017

SparkQA commented Jun 27, 2017

cloud-fan commented Jun 27, 2017

viirya commented Jun 26, 2017 •

edited

Loading