Skip to content

Conversation

@LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

The purpose of this PR is to resolve SPARK-32526 which should allow Spark to compile sql/catalyst module with Scala 2.13 profile.

The main changes of this pr is adding .toSeq calls where mutable collections are returned as Seq, similar as #28791 .

Why are the changes needed?

We need to support a Scala 2.13 build.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests. (2.13 was not tested, there are still about 100 failed test cases with 2.13 profile in sql/catalyst module.)

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 4, 2020

Thank you for making a PR, @LuciferYang . However, the compilation is completed by @srowen via SPARK-29292.
Since there is no CI coverage, Scala 2.13 compilation and UT is going to be broken time to time.
This is a know issue and we already discussed. The decision is to fix it again when we make this module's UT pass. For example, SPARK-32489. Could you try to pass sql/catalyst UTs in this PR?

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Aug 4, 2020

@dongjoon-hyun Got it ~ there are about 100 test case failed of sql/catalyst module, I will try to fix these in this pr

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 4, 2020

Thanks. You don't need to fix everything in one shot. Please group the failures and fix the one you think this is related to many others. You can proceed by suite by suite.

@LuciferYang
Copy link
Contributor Author

OK ~

@srowen
Copy link
Member

srowen commented Aug 4, 2020

Yeah, certainly we need to make these fixes to newly-arrived code over time, but I'd just do them when fixing a chunk of tests.

@srowen srowen closed this Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants