[SPARK-25371][SQL] struct() should allow being called with 0 args #22373

mgaido91 · 2018-09-09T13:11:10Z

What changes were proposed in this pull request?

SPARK-21281 introduced a check for the inputs of CreateStructLike to be non-empty. This means that struct(), which was previously considered valid, now throws an Exception. This behavior change was introduced in 2.3.0. The change may break users' application on upgrade and it causes VectorAssembler to fail when an empty inputCols is defined.

The PR removes the added check making struct() valid again.

How was this patch tested?

added UT

mgaido91 · 2018-09-09T13:11:31Z

cc @dongjoon-hyun @gatorsmile @maropu who worked on SPARK-21281

SparkQA · 2018-09-09T14:21:52Z

Test build #95847 has finished for PR 22373 at commit 7bc026e.

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

dongjoon-hyun · 2018-09-10T03:44:34Z

mllib/src/test/scala/org/apache/spark/ml/feature/VectorAssemblerSuite.scala

@@ -256,4 +256,9 @@ class VectorAssemblerSuite
    assert(runWithMetadata("keep", additional_filter = "id1 > 2").count() == 4)
  }

+  test("SPARK-25371: VectorAssembler with empty inputCols") {
+    val vectorAssembler = new VectorAssembler().setInputCols(Array()).setOutputCol("a")


Is VectorAssembler with zero input column useful?

cc @yanboliang and @srowen

It doesn't sound that useful, but the JIRA suggests this is the behavior in 2.2. It throws a weird error in 2.3. I could imagine just allowing this behavior, or throwing a better exception. Is there a use case for no input? maybe you have some reusable pipeline that is applied to a subset of columns and sometimes it matches nothing. The output is empty but maybe that doesn't matter for whatever purpose it serves... maybe it's assembled with something else afterwards. I could picture a valid use case.

HyukjinKwon · 2018-09-10T08:11:57Z

@mgaido91, BTW are you sure SPARK-21281 introduced that behaviour change?

Before:

scala> import org.apache.spark.sql.functions.struct
import org.apache.spark.sql.functions.struct

scala> spark.range(1).select(struct()).show()
org.apache.spark.sql.AnalysisException: cannot resolve 'named_struct()' due to data type mismatch: input to function named_struct requires at least one argument;;
'Project [unresolvedalias(named_struct(), None)]
+- Range (0, 1, step=1, splits=Some(8))

After:

scala> import org.apache.spark.sql.functions.struct
import org.apache.spark.sql.functions.struct

scala> spark.range(1).select(struct()).show()
java.lang.AssertionError: assertion failed: each serializer expression should contain at least one `BoundReference`
  at scala.Predef$.assert(Predef.scala:170)

it should be good to document that behaviour change if that's allowed before in a separate PR.

mgaido91 · 2018-09-10T08:53:09Z

@HyukjinKwon I am sure, since I tried removing the added check and the UT I added here passed.

mgaido91 · 2018-09-10T10:47:23Z

cc @cloud-fan @jerryshao despite a very minor one, this can be considered a regression, so may be considered as a blocker for 2.4/2.3.2?

cloud-fan · 2018-09-10T11:50:31Z

I think we should allow struct function to take empty arguments.

mgaido91 · 2018-09-10T11:57:05Z

I think we should allow struct function to take empty arguments.

Yes, I agree @cloud-fan. At least until 3.0 IMHO. But since the change was already released in 3.0, I was not sure whether to revert that additional check or add this logic here. If nobody objects, I'd switch to remove the check, since this would also prevent other users's workflows to potentially break having a regression.

cloud-fan · 2018-09-10T12:29:06Z

@maropu says it's OK to revert that part, @mgaido91 can you do that? thanks!

mgaido91 · 2018-09-10T12:32:30Z

thanks @cloud-fan @maropu , I'll update this accordingly ASAP, thanks

maropu · 2018-09-10T12:34:38Z

ya, sorry for bothering you all. thansk, @mgaido91

cloud-fan · 2018-09-10T13:35:16Z

mllib/src/test/scala/org/apache/spark/ml/feature/VectorAssemblerSuite.scala

@@ -256,4 +256,9 @@ class VectorAssemblerSuite
    assert(runWithMetadata("keep", additional_filter = "id1 > 2").count() == 4)
  }

+  test("SPARK-25371: VectorAssembler with empty inputCols") {


Do we still need this test?

I think so, after this patch this test passes, before it doesn't. I think it is helpful to avoid regressions like this in the future.

cloud-fan · 2018-09-10T13:35:35Z

Can you also update the PR description? thanks!

SparkQA · 2018-09-10T19:25:05Z

Test build #95877 has finished for PR 22373 at commit 69ff3cb.

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

mgaido91 · 2018-09-10T19:34:46Z

retest this please

SparkQA · 2018-09-10T23:46:34Z

Test build #95895 has finished for PR 22373 at commit 69ff3cb.

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

dongjoon-hyun

+1, LGTM.

## What changes were proposed in this pull request? SPARK-21281 introduced a check for the inputs of `CreateStructLike` to be non-empty. This means that `struct()`, which was previously considered valid, now throws an Exception. This behavior change was introduced in 2.3.0. The change may break users' application on upgrade and it causes `VectorAssembler` to fail when an empty `inputCols` is defined. The PR removes the added check making `struct()` valid again. ## How was this patch tested? added UT Closes #22373 from mgaido91/SPARK-25371. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 0736e72) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2018-09-11T06:19:53Z

thanks, merging to master/2.4!

@mgaido91 can you send a new PR to 2.3? it conflicts

SPARK-21281 introduced a check for the inputs of `CreateStructLike` to be non-empty. This means that `struct()`, which was previously considered valid, now throws an Exception. This behavior change was introduced in 2.3.0. The change may break users' application on upgrade and it causes `VectorAssembler` to fail when an empty `inputCols` is defined. The PR removes the added check making `struct()` valid again. added UT Closes apache#22373 from mgaido91/SPARK-25371. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

HyukjinKwon

LGTM too

SPARK-21281 introduced a check for the inputs of `CreateStructLike` to be non-empty. This means that `struct()`, which was previously considered valid, now throws an Exception. This behavior change was introduced in 2.3.0. The change may break users' application on upgrade and it causes `VectorAssembler` to fail when an empty `inputCols` is defined. The PR removes the added check making `struct()` valid again. added UT Closes apache#22373 from mgaido91/SPARK-25371. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 0736e72) RB=1520856 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli

[SPARK-25371][SQL] VectorAssembler shpuld not fail with empty inputCols

7bc026e

dongjoon-hyun reviewed Sep 10, 2018

View reviewed changes

cloud-fan mentioned this pull request Sep 10, 2018

[SPARK-21281][SQL] Use string types by default if array and map have no argument #18516

Closed

Revert check in struct for more than one argument

41bd5bd

mgaido91 changed the title ~~[SPARK-25371][ML] VectorAssembler should not fail with empty inputCols~~ [SPARK-25371][SQL] struct() should allow being called with 0 args Sep 10, 2018

fix imports

69ff3cb

cloud-fan reviewed Sep 10, 2018

View reviewed changes

dongjoon-hyun approved these changes Sep 11, 2018

View reviewed changes

asfgit closed this in 0736e72 Sep 11, 2018

HyukjinKwon reviewed Sep 11, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25371][SQL] struct() should allow being called with 0 args #22373

[SPARK-25371][SQL] struct() should allow being called with 0 args #22373

mgaido91 commented Sep 9, 2018 •

edited

Loading

mgaido91 commented Sep 9, 2018 •

edited

Loading

SparkQA commented Sep 9, 2018

dongjoon-hyun Sep 10, 2018

dongjoon-hyun Sep 10, 2018

srowen Sep 10, 2018

HyukjinKwon commented Sep 10, 2018 •

edited

Loading

mgaido91 commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

cloud-fan commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

cloud-fan commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

maropu commented Sep 10, 2018 •

edited

Loading

cloud-fan Sep 10, 2018

mgaido91 Sep 10, 2018

cloud-fan commented Sep 10, 2018

SparkQA commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

SparkQA commented Sep 10, 2018

dongjoon-hyun left a comment

cloud-fan commented Sep 11, 2018

HyukjinKwon left a comment

[SPARK-25371][SQL] struct() should allow being called with 0 args #22373

[SPARK-25371][SQL] struct() should allow being called with 0 args #22373

Conversation

mgaido91 commented Sep 9, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

mgaido91 commented Sep 9, 2018 • edited Loading

SparkQA commented Sep 9, 2018

dongjoon-hyun Sep 10, 2018

Choose a reason for hiding this comment

dongjoon-hyun Sep 10, 2018

Choose a reason for hiding this comment

srowen Sep 10, 2018

Choose a reason for hiding this comment

HyukjinKwon commented Sep 10, 2018 • edited Loading

mgaido91 commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

cloud-fan commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

cloud-fan commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

maropu commented Sep 10, 2018 • edited Loading

cloud-fan Sep 10, 2018

Choose a reason for hiding this comment

mgaido91 Sep 10, 2018

Choose a reason for hiding this comment

cloud-fan commented Sep 10, 2018

SparkQA commented Sep 10, 2018

mgaido91 commented Sep 10, 2018

SparkQA commented Sep 10, 2018

dongjoon-hyun left a comment

Choose a reason for hiding this comment

cloud-fan commented Sep 11, 2018

HyukjinKwon left a comment

Choose a reason for hiding this comment

mgaido91 commented Sep 9, 2018 •

edited

Loading

mgaido91 commented Sep 9, 2018 •

edited

Loading

HyukjinKwon commented Sep 10, 2018 •

edited

Loading

maropu commented Sep 10, 2018 •

edited

Loading