[SPARK-34141][SQL] Remove side effect from ExtractGenerator by tanelk · Pull Request #31213 · apache/spark

tanelk · 2021-01-17T00:08:55Z

What changes were proposed in this pull request?

Rewrote one ExtractGenerator case such that it would not rely on a side effect of the flatmap function.

Why are the changes needed?

With the dataframe api it is possible to have a lazy sequence as the output of a LogicalPlan. When exploding a column on this dataframe using the withColumn("newName", explode(col("name"))) method, the ExtractGenerator does not extract the generator and CheckAnalysis would throw an exception.

Does this PR introduce any user-facing change?

Bugfix
Before this, the work around was to put .select("*") before the explode.

How was this patch tested?

UT

tanelk · 2021-01-17T00:09:54Z

@HyukjinKwon , any chance that this bugfix could get to 3.1.1?

SparkQA · 2021-01-17T01:34:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38737/

SparkQA · 2021-01-17T02:01:43Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38737/

SparkQA · 2021-01-17T05:40:59Z

Test build #134154 has finished for PR 31213 at commit 08e367c.

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

SparkQA · 2021-01-17T07:43:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38741/

tanelk · 2021-01-17T08:17:33Z

@LuciferYang, you have been working with 2.13.
Any recommendations on how to get a lazy Seq on both 2.12 and 2.13.
On 2.13 the Seq.view does not return a Seq anymore.

SparkQA · 2021-01-17T08:32:51Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38741/

SparkQA · 2021-01-17T11:32:37Z

Test build #134158 has finished for PR 31213 at commit cefc79f.

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

LuciferYang · 2021-01-18T03:35:51Z

@tanelk In Scala 2.13, Views are collections whose transformation operations are non strict and they no longer extend their corresponding collection type, for example, an IndexedSeqView no longer extends IndexedSeq.

Why do we need a non strict collection here? Can it be a strict collection?
If it must be a non strict collection, may need to define the type of output as scala.collection.SeqView for version compatibility.

tanelk · 2021-01-18T04:07:50Z

Well that is the test case I'm trying to cover.
in 2.12 the input can be lazy Seq, but the current code breaks then.
Perhaps it's enough to say that I manually tested it and not add the UT.

LuciferYang · 2021-01-18T05:19:40Z

cc @srowen

LuciferYang · 2021-01-19T02:47:22Z

Well that is the test case I'm trying to cover.
in 2.12 the input can be lazy Seq, but the current code breaks then.
Perhaps it's enough to say that I manually tested it and not add the UT.

New UT in this pr will break the compilation of Scala 2.13, @tanelk want to get a lazy Seq on both 2.12 and 2.13, but SeqView is not Seq in Scala 2.13, do you have any suggestions ? @srowen

srowen · 2021-01-19T13:07:30Z

Hm why does Seq vs SeqView matter here? what's the compile error?

LuciferYang · 2021-01-19T13:45:56Z

tanelk · 2021-01-19T13:46:41Z

@srowen
There is a correctness issue, when an lazy Seq is given as the project list (for example SeqView).
I did fix it, but the unit test does not compile on 2.13, because there SeqView does not implement Seq any more.

Would it be okay do not add the UT or is there some lazy Seq present on both 2.12 and 2.13.

srowen · 2021-01-19T16:57:57Z

Hm, tough call. Can you invoke the second constructor by adding :_* to the end of the argument? But then I suppose that's not testing what you want to test either. Can you pass a Scala Stream here maybe? not sure if that works. It may work to create a new test-suite in a 2.12-only src/test directory if that's not too much trouble. It sounds like this can't happen in 2.13 anyway so the test isn't relevant.

SparkQA · 2021-01-26T17:24:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39096/

SparkQA · 2021-01-26T17:54:51Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39096/

tanelk · 2021-01-26T18:47:14Z

I went ahead and added version dependent tests. Thanks for the advice.

srowen · 2021-01-26T18:51:35Z

...alyst/src/test/scala-2.13/org/apache/spark/sql/catalyst/analysis/ExtractGeneratorSuite.scala

+import org.apache.spark.sql.catalyst.plans.logical._
+import org.apache.spark.sql.types._
+
+class ExtractGeneratorSuite extends AnalysisTest {


I'd add comments in each test file noting that there is a parallel one in the other source tree, so people realize that both need to change.

If you'll add the comments I'll merge it @tanelk

SparkQA · 2021-01-26T21:17:03Z

Test build #134511 has finished for PR 31213 at commit 8dd6da0.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ExtractGeneratorSuite extends AnalysisTest
class ExtractGeneratorSuite extends AnalysisTest

SparkQA · 2021-01-31T08:27:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39275/

SparkQA · 2021-01-31T08:54:41Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39275/

SparkQA · 2021-01-31T11:45:34Z

Test build #134689 has finished for PR 31213 at commit 82ca8f5.

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

maropu · 2021-02-01T06:45:51Z

...alyst/src/test/scala-2.12/org/apache/spark/sql/catalyst/analysis/ExtractGeneratorSuite.scala

+    val explode = Alias(Explode(b), "c")()
+
+    // view is a lazy seq
+    val rel = LocalRelation(output = columns.view)


Is it possible for end users to get stuck on this issue? If possible, could you add end-2-end tests, too?

I hit this issue while using spark in java. The step before was an join, where I used JavaConverters.collectionAsScalaIterable(Arrays.asList(columns)).toSeq() as the join condition. The JavaConverters helper returns an lazy collection.
I can take a look at on how to e2e test this.

srowen · 2021-02-06T19:27:25Z

Merged to master. I'm satisfied with the motivation and test.

HyukjinKwon · 2021-04-06T06:18:21Z

Oh, just saw it. LGTM

Remove side effect

08e367c

github-actions bot added the SQL label Jan 17, 2021

2.13

cefc79f

srowen approved these changes Jan 18, 2021

View reviewed changes

Version specific tests

8dd6da0

srowen reviewed Jan 26, 2021

View reviewed changes

Add comments

82ca8f5

maropu reviewed Feb 1, 2021

View reviewed changes

srowen closed this in c73f70b Feb 6, 2021

Conversation

tanelk commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tanelk commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

tanelk commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

LuciferYang commented Jan 18, 2021

Uh oh!

tanelk commented Jan 18, 2021

Uh oh!

LuciferYang commented Jan 18, 2021

Uh oh!

LuciferYang commented Jan 19, 2021

Uh oh!

srowen commented Jan 19, 2021

Uh oh!

LuciferYang commented Jan 19, 2021

Uh oh!

tanelk commented Jan 19, 2021

Uh oh!

srowen commented Jan 19, 2021

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

tanelk commented Jan 26, 2021

Uh oh!

srowen Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

srowen Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

SparkQA commented Jan 31, 2021

Uh oh!

SparkQA commented Jan 31, 2021

Uh oh!

SparkQA commented Jan 31, 2021

Uh oh!

maropu Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

tanelk Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

maropu Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

srowen commented Feb 6, 2021

Uh oh!

HyukjinKwon commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

tanelk commented Jan 17, 2021 •

edited

Loading