-
Notifications
You must be signed in to change notification settings - Fork 29k
SPARK-38661: [TESTS][FOLLOWUP] Replace Symbol usage with $"" in unit tests #35976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARK-38661: [TESTS][FOLLOWUP] Replace Symbol usage with $"" in unit tests #35976
Conversation
|
Thanks @martin-g ! this is larger than the original change, so is it safe to assume that this implements |
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystTypeConvertersSuite.scala
Outdated
Show resolved
Hide resolved
|
Can one of the admins verify this patch? |
This PR also changes There are more tests which still use |
…tests Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> (cherry picked from commit a7551b8)
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> (cherry picked from commit 1fd14242f9b8068ab380f3f290f406e8993eeb9a)
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
srowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did skim all of it (wow, big change) and there are maybe a few things we need to fix, but mostly looking good
core/src/test/scala/org/apache/spark/metrics/sink/PrometheusServletSuite.scala
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/PredicateSuite.scala
Outdated
Show resolved
Hide resolved
sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/HiveOrcQuerySuite.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Copied-from: #36029 Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
|
@srowen @HeartSaVioR @awdavidson I think the PR is ready for review now! |
|
It's looking OK, let's check tests. I'm slightly uneasy because it's hard to review every line of change, though I think we've skimmed them all. Do you feel pretty good that there aren't other typos? |
|
I echo @srowen pretty big change but generally looking ok. Let’s address test failure Not to increase the scope of this change but a follow up could be to add a scalastyle check warning against the use of |
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
I've checked all changes and they look OK to me ! The last test failure was because |
|
Merged to master |
|
Thank you for your help, @srowen ! |
What changes were proposed in this pull request?
Replace symbols like 'abc and Symbol("abc") (where possible) with the more verbose $"abc" in the test code.
Why are the changes needed?
This is a follow-up of #35560
Building with Scala 2.13 produces a lot of warnings like the following ones:
This should make it easier to upgrade to Scala 3 later.
Does this PR introduce any user-facing change?
No! The PR touches only test classes!
How was this patch tested?
The build at CI must be green!