Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Jun 25, 2021

What changes were proposed in this pull request?

This PR proposes to add a new scalastyle rule to enforce not importing scala.collection.Seq and scala.collection.IndexedSeq which conflicts with scala.Seq and scala.IndexedSeq.

The problem occurs as Scala 2.13 changed the alias of scala.Seq and scala.IndexedSeq. Before Scala 2.13, they were scala.collection.Seq and scala.collection.IndexedSeq. After Scala 2.13, they become scala.collection.immutable.Seq and scala.collection.immutable.IndexedSeq.

Please refer below doc for more details.
https://docs.scala-lang.org/overviews/core/collections-migration-213.html

Why are the changes needed?

We have seen Seq/IndexedSeq issues on cross-compilation of Scala 2.12 / 2.13. While I'm not sure this can prevent all cases, this will prevent the simple case of breaking cross compilation.

Does this PR introduce any user-facing change?

No change on end user. Contributors will be restricted but shouldn't block them doing the right thing.

How was this patch tested?

Ran scalastyle against current master (before #33084)

> dev/scalastyle
Scalastyle checks failed at following occurrences:
[error] /Users/Jungtaek.Lim/WorkArea/ScalaProjects/spark-apache/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:28:0:
[error]       Don't import scala.collection.Seq and scala.collection.IndexedSeq as it may bring some problems with cross-build between Scala 2.12 and 2.13.
[error]
[error]       Please refer below page to see the details of changes around Seq.
[error]       https://docs.scala-lang.org/overviews/core/collections-migration-213.html
[error]
[error]       If you really need to use scala.collection.Seq or scala.collection.IndexedSeq, please use the fully-qualified name instead.
[error]
[error] /Users/Jungtaek.Lim/WorkArea/ScalaProjects/spark-apache/core/src/main/scala/org/apache/spark/util/Utils.scala:37:0:
[error]       Don't import scala.collection.Seq and scala.collection.IndexedSeq as it may bring some problems with cross-build between Scala 2.12 and 2.13.
[error]
[error]       Please refer below page to see the details of changes around Seq.
[error]       https://docs.scala-lang.org/overviews/core/collections-migration-213.html
[error]
[error]       If you really need to use scala.collection.Seq or scala.collection.IndexedSeq, please use the fully-qualified name instead.
[error]
[error] Total time: 15 s, completed Jun 25, 2021 9:01:32 PM

@github-actions github-actions bot added the BUILD label Jun 25, 2021
@HeartSaVioR
Copy link
Contributor Author

HeartSaVioR commented Jun 25, 2021

Build failed by intention.

https://github.com/HeartSaVioR/spark/runs/2913871777?check_suite_focus=true

It reports scalastyle failure on master branch:

[error] /home/runner/work/spark/spark/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:28:0: 
[error]       Don't import scala.collection.Seq and scala.collection.IndexedSeq as it may bring some problems with cross-build between Scala 2.12 and 2.13.
[error] 
[error]       Please refer below page to see the details of changes around Seq / IndexedSeq.
[error]       https://docs.scala-lang.org/overviews/core/collections-migration-213.html
[error] 
[error]       If you really need to use scala.collection.Seq or scala.collection.IndexedSeq, please use the fully-qualified name instead.
[error]     
[error] /home/runner/work/spark/spark/core/src/main/scala/org/apache/spark/util/Utils.scala:37:0: 
[error]       Don't import scala.collection.Seq and scala.collection.IndexedSeq as it may bring some problems with cross-build between Scala 2.12 and 2.13.
[error] 
[error]       Please refer below page to see the details of changes around Seq / IndexedSeq.
[error]       https://docs.scala-lang.org/overviews/core/collections-migration-213.html
[error] 
[error]       If you really need to use scala.collection.Seq or scala.collection.IndexedSeq, please use the fully-qualified name instead.

The fix of above things would happen in different PR, #33084

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

Test build #140324 has finished for PR 33085 at commit c93588a.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HeartSaVioR
Copy link
Contributor Author

cc. @srowen as I see he had been taking care of Scala things.

@srowen
Copy link
Member

srowen commented Jun 25, 2021

Probably a reasonable idea. What changes are needed to comply with the rule - are there lots?

@HeartSaVioR
Copy link
Contributor Author

No, just two lines for now as the style check caught above in PR description. #33084 will handle the fix.

@srowen
Copy link
Member

srowen commented Jun 25, 2021

OK let's merge that and retest this

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

Test build #140325 has finished for PR 33085 at commit 46929da.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@HeartSaVioR
Copy link
Contributor Author

Rebased to contain #33084 , and fixed one missed spot in this PR. style check should pass now.

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2021

Test build #140327 has finished for PR 33085 at commit fc29c1a.

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

@HeartSaVioR
Copy link
Contributor Author

GA build passed for Scala 2.13 build, and style check with new rule is now passed.

@srowen Would it be good to go?

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go for it, LGTM

@HeartSaVioR
Copy link
Contributor Author

Thanks, merging to master!

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.

4 participants