Skip to content

Conversation

@LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

There are some compilation warnings in Scala 2.13 as follows:

[WARNING] [Warn] /spark-source/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala:88: [deprecation @  | origin= | version=2.13.0] symbol literal is deprecated; use Symbol("arr") instead
[WARNING] [Warn] /spark-source/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala:102: [deprecation @  | origin= | version=2.13.0] symbol literal is deprecated; use Symbol("arr") instead

The main change of this pr is add a compile arg to temporarily suppress these compilation warnings to make compile log look cleaner.

Why are the changes needed?

Temporarily suppress symbol literal is deprecated compilation warnings to make compile log look cleaner.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Pass the Jenkins or GitHub Action
  • Manual test:symbol literal is deprecated related warnings are no longer included in the compilation log

@LuciferYang
Copy link
Contributor Author

cc @cloud-fan

@github-actions github-actions bot added the BUILD label Apr 20, 2021
cloud-fan
cloud-fan previously approved these changes Apr 20, 2021
@SparkQA
Copy link

SparkQA commented Apr 20, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 20, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 20, 2021

Test build #137694 has finished for PR 32261 at commit 4c97fc9.

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

<arg>-Wconf:msg=Auto-application to \`\(\)\` is deprecated:s</arg>
<arg>-Wconf:msg=method with a single empty parameter list overrides method without any parameter list:s</arg>
<arg>-Wconf:msg=method without a parameter list overrides a method with a single empty one:s</arg>
<arg>-Wconf:msg=symbol literal is deprecated:s</arg>
Copy link
Member

@HyukjinKwon HyukjinKwon Apr 21, 2021

Choose a reason for hiding this comment

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

Can we have any feedback before we ignoring the symbol literal deprecations?

Copy link
Contributor Author

@LuciferYang LuciferYang Apr 21, 2021

Choose a reason for hiding this comment

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

@HyukjinKwon I think this may be a compromise for #31569 if we don't plan to fix it recently. I just asked in #31569, there is no other feedback for the time being except for @cloud-fan.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the only choice for now as we don't want to remove the symbol usage in Spark. We can rely on the Scala feature import and ask the Scala community to keep it forever. If it's removed in the future, we can change the Spark code at that time.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, if we're going to keep them explicitly, I'm fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are new updates in #31569 ...

Copy link
Member

Choose a reason for hiding this comment

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

Wait, I'm still not clear why we can't replace 'foo with Symbol(foo). That does not remove usage of Symbol. It just removes usage of the deprecated literal syntax. I thought the resolution was that this much was fine? the other issue is kind of conflating the deprecated syntax and Symbol itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

We use symbol literal in our testing DSL because it's easy to write. If we need to write Symbol("foo") from now on, I'd vote for updating the test DSL and pick a different syntax, such as #31601

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's not as compact. I'm OK with just suppressing it too, if the code churn is too much of a PITA.

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Test build #138038 has finished for PR 32261 at commit 56ba63c.

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

@LuciferYang LuciferYang reopened this May 10, 2021
@SparkQA
Copy link

SparkQA commented May 10, 2021

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

@SparkQA
Copy link

SparkQA commented May 10, 2021

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

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Aug 19, 2021
@github-actions github-actions bot closed this Aug 20, 2021
@LuciferYang LuciferYang deleted the SPARK-35151 branch June 6, 2022 03:47
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