Skip to content

Conversation

@HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR is basically a followup of #14332.
Calling map alone might leave it not executed due to lazy evaluation, e.g.)

scala> val foo = Seq(1,2,3)
foo: Seq[Int] = List(1, 2, 3)

scala> foo.map(println)
1
2
3
res0: Seq[Unit] = List((), (), ())

scala> foo.view.map(println)
res1: scala.collection.SeqView[Unit,Seq[_]] = SeqViewM(...)

scala> foo.view.foreach(println)
1
2
3

We should better use foreach to make sure it's executed where the output is unused or Unit.

Why are the changes needed?

To prevent the potential issues by not executing map.

Does this PR introduce any user-facing change?

No, the current codes look not causing any problem for now.

How was this patch tested?

I found these item by running IntelliJ inspection, double checked one by one, and fixed them. These should be all instances across the codebase ideally.

@HyukjinKwon
Copy link
Member Author

cc @srowen FYI

@SparkQA
Copy link

SparkQA commented Jan 10, 2021

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

@SparkQA
Copy link

SparkQA commented Jan 10, 2021

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

@SparkQA

This comment has been minimized.

@SparkQA
Copy link

SparkQA commented Jan 10, 2021

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

@SparkQA
Copy link

SparkQA commented Jan 10, 2021

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

@SparkQA
Copy link

SparkQA commented Jan 10, 2021

Test build #133890 has finished for PR 31110 at commit 74bc183.

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

@viirya
Copy link
Member

viirya commented Jan 10, 2021

Thanks! Merging to master.

@viirya viirya closed this in 8302492 Jan 10, 2021
@HyukjinKwon
Copy link
Member Author

Thanks @srowen and @viirya!

@mridulm
Copy link
Contributor

mridulm commented Jan 11, 2021

Thanks for fixing this @HyukjinKwon ! Surprised these did not cause problems earlier.
Do we want to backport this to older branches as well ?

@HyukjinKwon
Copy link
Member Author

Sure I'll prepare backport

@srowen
Copy link
Member

srowen commented Jan 11, 2021

It will definitely have the same effect if the collection is not lazy, so I doubt it currently is the cause of a bug. But yeah it'd be fine to backport in case there is any chance the collection that something returns suddenly gets lazy.

HyukjinKwon added a commit that referenced this pull request Jan 12, 2021
… sure execute it eagerly

### What changes were proposed in this pull request?

This is a backport of #31110. I ran intelliJ inspection again in this branch.

This PR is basically a followup of #14332.
Calling `map` alone might leave it not executed due to lazy evaluation, e.g.)

```
scala> val foo = Seq(1,2,3)
foo: Seq[Int] = List(1, 2, 3)

scala> foo.map(println)
1
2
3
res0: Seq[Unit] = List((), (), ())

scala> foo.view.map(println)
res1: scala.collection.SeqView[Unit,Seq[_]] = SeqViewM(...)

scala> foo.view.foreach(println)
1
2
3
```

We should better use `foreach` to make sure it's executed where the output is unused or `Unit`.

### Why are the changes needed?

To prevent the potential issues by not executing `map`.

### Does this PR introduce _any_ user-facing change?

No, the current codes look not causing any problem for now.

### How was this patch tested?

I found these item by running IntelliJ inspection, double checked one by one, and fixed them. These should be all instances across the codebase ideally.

Closes #31137 from HyukjinKwon/SPARK-34059-3.1.

Authored-by: HyukjinKwon <gurwls223@apache.org>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
HyukjinKwon added a commit that referenced this pull request Jan 12, 2021
… sure execute it eagerly

### What changes were proposed in this pull request?

This is a backport of #31110. I ran intelliJ inspection again in this branch.

This PR is basically a followup of #14332.
Calling `map` alone might leave it not executed due to lazy evaluation, e.g.)

```
scala> val foo = Seq(1,2,3)
foo: Seq[Int] = List(1, 2, 3)

scala> foo.map(println)
1
2
3
res0: Seq[Unit] = List((), (), ())

scala> foo.view.map(println)
res1: scala.collection.SeqView[Unit,Seq[_]] = SeqViewM(...)

scala> foo.view.foreach(println)
1
2
3
```

We should better use `foreach` to make sure it's executed where the output is unused or `Unit`.

### Why are the changes needed?

To prevent the potential issues by not executing `map`.

### Does this PR introduce _any_ user-facing change?

No, the current codes look not causing any problem for now.

### How was this patch tested?

I found these item by running IntelliJ inspection, double checked one by one, and fixed them. These should be all instances across the codebase ideally.

Closes #31138 from HyukjinKwon/SPARK-34059-3.0.

Authored-by: HyukjinKwon <gurwls223@apache.org>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
HyukjinKwon added a commit that referenced this pull request Jan 12, 2021
… sure execute it eagerly

### What changes were proposed in this pull request?

This is a backport of #31110. I ran intelliJ inspection again in this branch.

This PR is basically a followup of #14332.
Calling `map` alone might leave it not executed due to lazy evaluation, e.g.)

```
scala> val foo = Seq(1,2,3)
foo: Seq[Int] = List(1, 2, 3)

scala> foo.map(println)
1
2
3
res0: Seq[Unit] = List((), (), ())

scala> foo.view.map(println)
res1: scala.collection.SeqView[Unit,Seq[_]] = SeqViewM(...)

scala> foo.view.foreach(println)
1
2
3
```

We should better use `foreach` to make sure it's executed where the output is unused or `Unit`.

### Why are the changes needed?

To prevent the potential issues by not executing `map`.

### Does this PR introduce _any_ user-facing change?

No, the current codes look not causing any problem for now.

### How was this patch tested?

I found these item by running IntelliJ inspection, double checked one by one, and fixed them. These should be all instances across the codebase ideally.

Closes #31139 from HyukjinKwon/SPARK-34059-2.4.

Authored-by: HyukjinKwon <gurwls223@apache.org>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
@HyukjinKwon HyukjinKwon deleted the SPARK-34059 branch January 4, 2022 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants