[SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations to comply with Scala 2.13 #31569

sarutak · 2021-02-16T02:54:56Z

What changes were proposed in this pull request?

This PR replaces all the occurrence of symbol literals with Symbol() constructors.

Why are the changes needed?

As of Scala 2.13, symbol literals are deprecated so when we build with Scala 2.13 and sbt, the compiler loudly inform us.

[warn] /home/kou/work/oss/spark-scala-2.13/examples/src/main/scala/org/apache/spark/examples/sql/SimpleTypedAggregator.scala:34:38: [deprecation @  | origin= | version=2.13.0] symbol literal is deprecated; use Symbol("id") instead
[warn]     val ds = spark.range(20).select(('id % 3).as("key"), 'id).as[(Long, Long)]
[warn]                                      ^
[warn] /home/kou/work/oss/spark-scala-2.13/examples/src/main/scala/org/apache/spark/examples/sql/SimpleTypedAggregator.scala:34:58: [deprecation @  | origin= | version=2.13.0] symbol literal is deprecated; use Symbol("id") instead
[warn]     val ds = spark.range(20).select(('id % 3).as("key"), 'id).as[(Long, Long)]

Does this PR introduce any user-facing change?

No.

How was this patch tested?

I confirmed that compile and test:compile successfully finish with both Scala 2.12 and 2.13.

sarutak · 2021-02-16T02:57:35Z

For reviewers: This PR changes too many files though the changes themselves are simple.
So if it's better, I'll split the PR.

SparkQA · 2021-02-16T04:03:11Z

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

HyukjinKwon · 2021-02-16T04:26:55Z

I am okay doing this in one go.

HyukjinKwon · 2021-02-16T04:28:57Z

cc @rxin and @srowen FYI (from Do you use single-quote syntax for the DataFrame API?)

SparkQA · 2021-02-16T05:10:13Z

Test build #135164 has started for PR 31569 at commit 25112ec.

SparkQA · 2021-02-16T05:16:39Z

Test build #135159 has finished for PR 31569 at commit be80a62.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-16T07:01:01Z

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

SparkQA · 2021-02-16T07:08:23Z

Test build #135166 has started for PR 31569 at commit 748bc9a.

SparkQA · 2021-02-16T07:19:22Z

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

srowen · 2021-02-16T15:08:44Z

I think it's ok. I did a lot of this a long time ago too just to tackle the compiler warnings.

srowen · 2021-02-16T18:37:09Z

I share some of that frustration, not to the same extreme. Breaking binary/source compatibility across every minor release is a ... painful choice for users to eat. I have always thought of Scala as a bit more of a research language and acts accordingly. There are upsides to deciding to change quickly. In this case - removing the weird back tick syntax is a good thing, but should it ever have been that way? But not sure why it should go away entirely. As one of the major Scala projects, I'd hope nothing happens that really busts how Spark has to work.

rxin · 2021-02-16T18:46:12Z

This is going to be a nightmare. I didn't realize this change had happened upstream in Scala 2.13. I felt like it'd be virtually impossible for most Spark users, including all Databricks customers, to upgrade in the future, because they would need to rewrite a lot of their code, and worse dependency code that they might not control.

sarutak · 2021-02-17T02:13:19Z

Hmm, sounds like bad.
This PR itself doesn't intend to prohibit users to use symbol literals (just suppress compiler warning) but if we'll complain to the Scala community of the change, I'll leave the code as it is.
What do you think?

HyukjinKwon · 2021-02-17T02:18:16Z

The change is fine but I have to say I feel frustrated either for the same reasons from @mallman, @srowen and @rxin above.

sarutak · 2021-02-17T02:59:04Z

retest this please.

SparkQA · 2021-02-17T03:16:26Z

Test build #135186 has started for PR 31569 at commit 748bc9a.

SparkQA · 2021-02-17T03:44:42Z

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

SparkQA · 2021-02-17T04:13:44Z

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

HeartSaVioR · 2021-02-18T01:22:21Z

Just a 2 cents, it might be better to convert to col() or $ (I'm now feeling unsafe for this as well though) instead of Symbol() on column representations as I feel like they're what we'd prefer as alternatives. Not a requirement as I know you should've done bugging works already.

maropu · 2021-02-18T02:58:52Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala


    // int type can be up cast to long type
-    val attrs1 = Seq('a.string, 'b.int)
+    val attrs1 = Seq(Symbol("a").string, Symbol("b").int)


If we replace this with another semantically-equal one, I prefer $"a" instead of Symbol as for the SQL module. IIRC @cloud-fan left the simillar comment in another PR.

…mples and documents ### What changes were proposed in this pull request? This PR replaces all the occurrences of symbol literals (`'name`) with string interpolation (`$"name"`) in examples and documents. ### Why are the changes needed? Symbol literals are used to represent columns in Spark SQL but the Scala community seems to remove `Symbol` completely. As we discussed in #31569, first we should replacing symbol literals with `$"name"` in user facing examples and documents. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Build docs. Closes #31615 from sarutak/replace-symbol-literals-in-doc-and-examples. Authored-by: Kousuke Saruta <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

rxin · 2021-02-26T01:25:17Z

I asked @dragos to chime in and he started a discussion with the Scala team and Martin. Let's see what happens.

SparkQA · 2021-04-06T14:16:20Z

Test build #136941 has finished for PR 31569 at commit 748bc9a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

LuciferYang · 2021-04-16T08:59:35Z

Is there any progress on this issue?

sarutak · 2021-04-16T09:50:45Z

@LuciferYang I keep watching the activity of the Scala community.
They will introduce a language import feature which allows to use symbol literals but it doesn't mean we can continue to use symbol literals forever.
lampepfl/dotty-feature-requests#182 (comment)

So, it seems that Spark users can use the language import feature until they finishes migrating their application but Spark needs to be replace symbol literals.
If we need to replace, most of them will be done by #31601

srowen · 2021-04-16T10:18:21Z

Backing up - this change isn't about removing usage of Symbol, but a deprecated syntax for Symbol. This doesn't actually change anything but how it's expressed in source code right? if so, why not do this in Spark (aside from the large code churn of course)?

Changing to Column syntax is another question.

sarutak · 2021-04-16T10:39:32Z

Backing up - this change isn't about removing usage of Symbol, but a deprecated syntax for Symbol. This doesn't actually change anything but how it's expressed in source code right? if so, why not do this in Spark (aside from the large code churn of course)?

I intended to just replace symbol literal with Symbol constructor invocation but I found Symbol is deprecated in the future.
#31569 (comment)
If the decision will not be changed, we need to remove the usage of symbols right? (If so, the title of this PR needs do be changed)

Changing to Column syntax is another question.

Yeah, at first, the syntax stuff is a separate one but after a discussion, the most part of the symbol usage was to be removed.
#31601 (comment)

srowen · 2021-04-16T11:03:23Z

That's right. That would indeed be a bigger, problematic change.

I'm saying that this change by itself isn't doing anything but removing deprecation warnings, so we're not arguing with it (except perhaps to debate the code churn).

If the argument is - we may have to make a bigger more painful change later anyway, so this isn't worth it, I buy that too. But this by itself seems plausible to merge.

SethTisue · 2021-04-16T13:54:01Z

fwiw, I will oppose any deprecation of Symbol itself. I don't think you should assume I will lose.

The literal syntax was worth deprecating because it decreases the amount of Scala syntax, which makes everything easier for tooling authors, educators, etc. Whereas deprecating Symbol itself would merely be annoying, to no great benefit.

sarutak · 2021-04-19T02:04:09Z

fwiw, I will oppose any deprecation of Symbol itself. I don't think you should assume I will lose.

The literal syntax was worth deprecating because it decreases the amount of Scala syntax, which makes everything easier for tooling authors, educators, etc. Whereas deprecating Symbol itself would merely be annoying, to no great benefit.

O.K, I don't care about replacing Symbol for now. Thank you.

cloud-fan · 2021-04-19T05:32:42Z

I think there are 2 topics here:

Public Spark APIs that use Symbols. We can't change them as they are public APIs. We are fine if Scala won't remove the Symbol class. I assume these public APIs will be less popular over time, as they are hard to use without the symbol literal syntax. $"col_name" is better.
Internal Spark APIs that use Symbols, especially the testing DSL. I'd prefer to not use Symbol anymore, as it becomes hard to use without the symbol literal syntax. e.g. LocalRelation('a.int) looks good, but LocalRelation(Symbol("a").int) looks cumbersome. We can create a new DSL syntax to make it easier to write tests in Spark. (see [SPARK-34484][SQL] Rename map to mapAttr in Catalyst DSL #31601)

What do you think?

sarutak · 2021-04-20T04:42:41Z

Public Spark APIs that use Symbols. We can't change them as they are public APIs. We are fine if Scala won't remove the Symbol class. I assume these public APIs will be less popular over time, as they are hard to use without the symbol literal syntax. $"col_name" is better.

Fortunately, the usage of Symbols in public non-internal APIs is limited.

as and alias in Dataset.scala
as in Column.scala
typedlit in functions.scala

So I think we can decide to deprecate these APIs later.

Internal Spark APIs that use Symbols, especially the testing DSL. I'd prefer to not use Symbol anymore, as it becomes hard to use without the symbol literal syntax. e.g. LocalRelation('a.int) looks good, but LocalRelation(Symbol("a").int) looks cumbersome. We can create a new DSL syntax to make it easier to write tests in Spark. (see #31601)

As I noticed in #31601, most usage of Symbols seems to be in testing DSL.
Symbol constructor is used a little but most is symbol literals.
So I think we can safely replace them with another syntax.

rxin · 2021-04-20T04:46:44Z

With all due respect, the number of functions is not a good measure. Those functions are the most fundamental ones in the DataFrame API. Basically any time anybody wants to refer to a column... just look up stackoverflow. There are tons of answers with sample code doing ‘col. I don’t think Scala should remove the symbol literal syntax given its damage. It looks like it’s possible it will be kept with an import and then Spark users wouldn’t need to suffer.

sarutak · 2021-04-20T04:56:10Z

DataFrame API. Basically any time anybody wants to refer to a column... just look up stack over low. There are tons of answers with sample code doing ‘col.

I mean that we don't need to decide how we treat those public APIs for now and it's easy even if we (unfortunately) need to replace it because there are three APIs.
If Scala community doesn't remove symbol literals, it's best.

It looks like it’s possible it will be kept with an import and then Spark users wouldn’t need to suffer.

Yes. As I mentioned here I think users can use the language import feature too.

cloud-fan · 2021-04-20T07:39:51Z

If the language import feature for symbol literal will be always there, I think we don't need any changes. We can probably wait until the Scala community decides to remove that language import.

LuciferYang · 2021-04-20T08:10:52Z

Or we can temporarily suppress the compilation warnings to make it look cleaner

cloud-fan · 2021-04-20T08:22:31Z

Or we can temporarily suppress the compilation warnings to make it look cleaner

+1

SparkQA · 2021-04-20T14:11:09Z

Test build #137686 has finished for PR 31569 at commit 748bc9a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

LuciferYang · 2021-04-20T14:43:58Z

@cloud-fan create a new jira SPARK-35151 (#32261)

SethTisue · 2021-04-21T00:55:39Z

If the language import feature for symbol literal will be always there

As I understand it, the language import was added to give the Spark project extra time to migrate; I don't think the intention was for Spark to then turn around and say "yay, now that we have this import we don't need to do anything".

cloud-fan · 2021-04-21T07:48:08Z

the language import was added to give the Spark project extra time to migrate

If that's the case, I think we need to change our internal DSL to not use symbol literals, which is done by #31601 . Shall we do it now or defer it? cc @rxin

smarter · 2021-04-29T14:09:04Z

If the language import feature for symbol literal will be always there,

It won't always be there, we've explicitly said that it can go away at any point: lampepfl/dotty-feature-requests#182 (comment).

srowen · 2021-04-29T14:49:39Z

Yup I think that if eventually it has to go away, then, maybe doesn't make sense to replace the deprecated syntax, just suppress warnings, and take it out entirely in the future all at once.

LuciferYang · 2021-07-01T02:36:34Z

Do we need to continue this work?

SparkQA · 2021-07-01T04:22:30Z

Test build #140475 has finished for PR 31569 at commit 748bc9a.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

github-actions · 2021-10-10T00:10:30Z

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!

Replace all symbol literals with Symbol constractors.

be80a62

github-actions bot added AVRO CORE EXAMPLES ML SQL STRUCTURED STREAMING labels Feb 16, 2021

sarutak changed the title ~~[SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations~~ [SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations to comply with Scala 2.13 Feb 16, 2021

Fix test.

25112ec

Fix test.

748bc9a

maropu reviewed Feb 18, 2021

View reviewed changes

LuciferYang mentioned this pull request Apr 21, 2021

[SPARK-35151][BUILD] Add compile arg to suppress symbol literal is deprecated compilation warnings in Scala 2.13 #32261

Closed

github-actions bot added the Stale label Oct 10, 2021

github-actions bot closed this Oct 11, 2021

HyukjinKwon mentioned this pull request Feb 22, 2022

[SPARK-38351][TESTS] Don't use deprecate symbol API in test classes #35560

Closed

[SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations to comply with Scala 2.13 #31569

[SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations to comply with Scala 2.13 #31569

Uh oh!

Conversation

sarutak commented Feb 16, 2021

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!

sarutak commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

HyukjinKwon commented Feb 16, 2021

Uh oh!

HyukjinKwon commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

SparkQA commented Feb 16, 2021

Uh oh!

srowen commented Feb 16, 2021

Uh oh!

srowen commented Feb 16, 2021

Uh oh!

rxin commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarutak commented Feb 17, 2021

Uh oh!

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

Uh oh!

Uh oh!

sarutak commented Feb 17, 2021

Uh oh!

SparkQA commented Feb 17, 2021

Uh oh!

SparkQA commented Feb 17, 2021

Uh oh!

SparkQA commented Feb 17, 2021

Uh oh!

HeartSaVioR commented Feb 18, 2021

Uh oh!

maropu Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Apr 6, 2021

Uh oh!

LuciferYang commented Apr 16, 2021

Uh oh!

sarutak commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Apr 16, 2021

Uh oh!

sarutak commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Apr 16, 2021

Uh oh!

SethTisue commented Apr 16, 2021

Uh oh!

sarutak commented Apr 19, 2021

Uh oh!

cloud-fan commented Apr 19, 2021

Uh oh!

sarutak commented Apr 20, 2021

Uh oh!

rxin commented Apr 20, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

rxin commented Feb 16, 2021 •

edited

Loading

HyukjinKwon commented Feb 17, 2021 •

edited

Loading

maropu Feb 18, 2021 •

edited

Loading

rxin commented Feb 26, 2021 •

edited

Loading

sarutak commented Apr 16, 2021 •

edited

Loading

sarutak commented Apr 16, 2021 •

edited

Loading

rxin commented Apr 20, 2021 via email •

edited

Loading

sarutak commented Apr 20, 2021 •

edited

Loading

LuciferYang commented Apr 20, 2021 •

edited

Loading