[SPARK-32167][SQL] Fix GetArrayStructFields to respect inner field's nullability together #28992

cloud-fan · 2020-07-03T08:30:18Z

What changes were proposed in this pull request?

Fix nullability of GetArrayStructFields. It should consider both the original array's containsNull and the inner field's nullability.

Why are the changes needed?

Fix a correctness issue.

Does this PR introduce any user-facing change?

Yes. See the added test.

How was this patch tested?

a new UT and end-to-end test

cloud-fan · 2020-07-03T08:30:45Z

cc @maropu @viirya @dongjoon-hyun

rednaxelafx

LGTM with a question inline

rednaxelafx · 2020-07-03T08:45:05Z

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

+  test("SPARK-32167: get field from an array of struct") {
+    val innerStruct = new StructType().add("i", "int", nullable = true)
+    val schema = new StructType().add("arr", ArrayType(innerStruct, containsNull = false))
+    val df = spark.createDataFrame(List(Row(Seq(Row(1), Row(null)))).asJava, schema)


A bit curious why asJava is needed here?

because the createDataFrame only takes java list, not scala Seq or List. We should probably fix that.

maropu

Nice catch! LGTM

HyukjinKwon

LGTM

gengliangwang

Nice catch!

viirya

Nice catch!

dongjoon-hyun

Thank you for the fix, @cloud-fan .

dongjoon-hyun

+1, LGTM. Thanks. I also verified locally.

SparkQA · 2020-07-05T23:14:04Z

Test build #124926 has finished for PR 28992 at commit bc3542a.

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

maropu · 2020-07-05T23:19:26Z

retest this please

dongjoon-hyun · 2020-07-05T23:26:30Z

Oh, it looks relevant. Could you check and update SelectedFieldSuite, @cloud-fan ?

org.scalatest.exceptions.TestFailedException: Expected 
StructField(col3,ArrayType(StructType(StructField(field1,StructType(StructField(subfield1,IntegerType,false)),true)),false),false), but got 
StructField(col3,ArrayType(StructType(StructField(field1,StructType(StructField(subfield1,IntegerType,false)),true)),true),false)

SparkQA · 2020-07-06T00:38:53Z

Test build #124978 has finished for PR 28992 at commit bc3542a.

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

SparkQA · 2020-07-06T11:31:05Z

Test build #125066 has finished for PR 28992 at commit eb81598.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-06T11:51:07Z

retest this please

SparkQA · 2020-07-06T15:33:58Z

Test build #125073 has finished for PR 28992 at commit eb81598.

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

cloud-fan · 2020-07-06T15:36:02Z

retest this please

dongjoon-hyun · 2020-07-06T15:36:14Z

Retest this please.

SparkQA · 2020-07-06T15:40:38Z

Test build #125099 has started for PR 28992 at commit eb81598.

shaneknapp · 2020-07-06T17:08:45Z

test this please

SparkQA · 2020-07-06T17:33:51Z

Test build #125106 has finished for PR 28992 at commit eb81598.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-06T18:46:33Z

Retest this please.

SparkQA · 2020-07-07T02:31:37Z

Test build #125115 has finished for PR 28992 at commit eb81598.

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

…nullability together ### What changes were proposed in this pull request? Fix nullability of `GetArrayStructFields`. It should consider both the original array's `containsNull` and the inner field's nullability. ### Why are the changes needed? Fix a correctness issue. ### Does this PR introduce _any_ user-facing change? Yes. See the added test. ### How was this patch tested? a new UT and end-to-end test Closes #28992 from cloud-fan/bug. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 5d296ed) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2020-07-07T03:09:26Z

Merged to master/3.0.
Could you make a backport to branch-2.4, @cloud-fan ?

…nullability together Fix nullability of `GetArrayStructFields`. It should consider both the original array's `containsNull` and the inner field's nullability. Fix a correctness issue. Yes. See the added test. a new UT and end-to-end test Closes apache#28992 from cloud-fan/bug. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 5d296ed) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ld's nullability together ### What changes were proposed in this pull request? Backport #28992 to 2.4 Fix nullability of `GetArrayStructFields`. It should consider both the original array's `containsNull` and the inner field's nullability. ### Why are the changes needed? Fix a correctness issue. ### Does this PR introduce _any_ user-facing change? Yes. See the added test. ### How was this patch tested? a new UT and end-to-end test Closes #29019 from cloud-fan/port. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

### What changes were proposed in this pull request? This PR aims to disable SBT `unidoc` generation testing in Jenkins environment because it's flaky in Jenkins environment and not used for the official documentation generation. Also, GitHub Action has the correct test coverage for the official documentation generation. - #28848 (comment) (amp-jenkins-worker-06) - #28926 (comment) (amp-jenkins-worker-06) - #28969 (comment) (amp-jenkins-worker-06) - #28975 (comment) (amp-jenkins-worker-05) - #28986 (comment) (amp-jenkins-worker-05) - #28992 (comment) (amp-jenkins-worker-06) - #28993 (comment) (amp-jenkins-worker-05) - #28999 (comment) (amp-jenkins-worker-04) - #29010 (comment) (amp-jenkins-worker-03) - #29013 (comment) (amp-jenkins-worker-04) - #29016 (comment) (amp-jenkins-worker-05) - #29025 (comment) (amp-jenkins-worker-04) - #29042 (comment) (amp-jenkins-worker-03) ### Why are the changes needed? Apache Spark `release-build.sh` generates the official document by using the following command. - https://github.com/apache/spark/blob/master/dev/create-release/release-build.sh#L341 ```bash PRODUCTION=1 RELEASE_VERSION="$SPARK_VERSION" jekyll build ``` And, this is executed by the following `unidoc` command for Scala/Java API doc. - https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L30 ```ruby system("build/sbt -Pkinesis-asl clean compile unidoc") || raise("Unidoc generation failed") ``` However, the PR builder disabled `Jekyll build` and instead has a different test coverage. ```python # determine if docs were changed and if we're inside the amplab environment # note - the below commented out until *all* Jenkins workers can get `jekyll` installed # if "DOCS" in changed_modules and test_env == "amplab_jenkins": # build_spark_documentation() ``` ``` Building Unidoc API Documentation ======================================================================== [info] Building Spark unidoc using SBT with these arguments: -Phadoop-3.2 -Phive-2.3 -Pspark-ganglia-lgpl -Pkubernetes -Pmesos -Phadoop-cloud -Phive -Phive-thriftserver -Pkinesis-asl -Pyarn unidoc ``` ### Does this PR introduce _any_ user-facing change? No. (This is used only for testing and not used in the official doc generation.) ### How was this patch tested? Pass the Jenkins without doc generation invocation. Closes #29017 from dongjoon-hyun/SPARK-DOC-GEN. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…nullability together Ref: LIHADOOP-56842 (cherry picked from commit 146062d) Backport apache#28992 to 2.4 Fix nullability of `GetArrayStructFields`. It should consider both the original array's `containsNull` and the inner field's nullability. Fix a correctness issue. Yes. See the added test. a new UT and end-to-end test Closes apache#29019 from cloud-fan/port. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> RB=2459030 BUG=LIHADOOP-56842 G=spark-reviewers R=zolin,ekrogen A=ekrogen

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

rednaxelafx reviewed Jul 3, 2020

View reviewed changes

maropu approved these changes Jul 3, 2020

View reviewed changes

HyukjinKwon approved these changes Jul 3, 2020

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-32167][SQL] fix nullability of GetArrayStructFields~~ [SPARK-32167][SQL] Fix GetArrayStructFields to respect inner field's nullability together Jul 3, 2020

gengliangwang approved these changes Jul 3, 2020

View reviewed changes

viirya approved these changes Jul 3, 2020

View reviewed changes

dongjoon-hyun reviewed Jul 4, 2020

View reviewed changes

dongjoon-hyun approved these changes Jul 4, 2020

View reviewed changes

fix nullability of GetArrayStructFields

eb81598

cloud-fan force-pushed the bug branch from bc3542a to eb81598 Compare July 6, 2020 11:06

dongjoon-hyun mentioned this pull request Jul 7, 2020

[SPARK-32233][TESTS] Disable SBT unidoc generation testing in Jenkins #29017

Closed

dongjoon-hyun closed this in 5d296ed Jul 7, 2020

cloud-fan mentioned this pull request Jul 7, 2020

[2.4][SPARK-32167][SQL] Fix GetArrayStructFields to respect inner field's nullability together #29019

Closed

[SPARK-32167][SQL] Fix GetArrayStructFields to respect inner field's nullability together #28992

[SPARK-32167][SQL] Fix GetArrayStructFields to respect inner field's nullability together #28992

Uh oh!

Conversation

cloud-fan commented Jul 3, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

cloud-fan commented Jul 3, 2020

Uh oh!

rednaxelafx left a comment

Choose a reason for hiding this comment

Uh oh!

rednaxelafx Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment • 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!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 5, 2020

Uh oh!

maropu commented Jul 5, 2020

Uh oh!

dongjoon-hyun commented Jul 5, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

maropu commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

cloud-fan commented Jul 6, 2020

Uh oh!

dongjoon-hyun commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

shaneknapp commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

dongjoon-hyun commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 7, 2020

Uh oh!

dongjoon-hyun commented Jul 7, 2020

Uh oh!

Uh oh!

dongjoon-hyun left a comment •

edited

Loading