You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/17 22:26:27 UTC
[GitHub] [spark] martin-g opened a new pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
martin-g opened a new pull request #35560:
URL: https://github.com/apache/spark/pull/35560
This PR replaces the deprecated `'sym` API with `Symbol("sym")` in the unit tests. This should make it easier to upgrade to Scala 3 later.
The PR is not finished!
I haven't reviewed it myself whether the Regexes I used didn't break something but the compilation passed locally.
With this PR I want to check what is Spark devs' opinion on this kind of change.
If it is approved I will create a ticket in JIRA and finish it!
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1052205509
If you want to rebase this and mark it ready for review, I think it could be OK. A good scope might be all occurrences in tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1055107188
@srowen The PR is ready for review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047332517
There was a previous try to refer to (https://github.com/apache/spark/pull/31569). I agree that we should probably make it go away anyway .. cc @sarutak FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058516997
Hm, I think you have a point. We definitely need Symbol in some parts of the Scala code, but I think they're rare and limited to parts that reason about closures, etc. For columns, yeah, in hindsight I agree.
Eh, hm, martin this might be my fault here. It may be smarter to turn all of those changes into $"..." after all if they're all really column refs. I totally missed that. We could do it in a follow up or revert this or whatever's easy, if you're willing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1053943535
I will create a JIRA ticket and update the PR soon!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1054608879
The build fails due to :
```
java.lang.RuntimeException: Failing because of negative scalastyle result
[error] at scala.sys.package$.error(package.scala:30)
[error] at org.scalastyle.sbt.Tasks$.handleResult$1(Plugin.scala:132)
[error] at org.scalastyle.sbt.Tasks$.doScalastyleWithConfig$1(Plugin.scala:187)
...
```
```
[error] /__w/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingJoinSuite.scala:580: File line length exceeds 100 characters
[error] /__w/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingJoinSuite.scala:927: File line length exceeds 100 characters
...
```
But executing `./dev/scalafmt` leads to many other unrelated formatting changes like:
```diff
import org.apache.spark.sql.catalyst.expressions.{InSet, Literal, NamedExpression}
-import org.apache.spark.sql.catalyst.util.DateTimeTestUtils.{outstandingTimezonesIds, outstandingZoneIds}
+import org.apache.spark.sql.catalyst.util.DateTimeTestUtils.{
+ outstandingTimezonesIds,
+ outstandingZoneIds
+}
import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.execution.ProjectExec
import org.apache.spark.sql.functions._
@@ -43,83 +46,54 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
import testImplicits._
private lazy val booleanData = {
- spark.createDataFrame(sparkContext.parallelize(
- Row(false, false) ::
- Row(false, true) ::
- Row(true, false) ::
- Row(true, true) :: Nil),
+ spark.createDataFrame(
+ sparkContext.parallelize(
+ Row(false, false) ::
+ Row(false, true) ::
+ Row(true, false) ::
+ Row(true, true) :: Nil),
```
It might take me a while to format manually just the related code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1079401056
@srowen https://github.com/apache/spark/pull/35976
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1044534254
@dongjoon-hyun @HyukjinKwon What is your opinion ? Would you merge such PR ?
How big should the PR be ? Fix all issues of this type in the whole or module by module ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon removed a comment on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
HyukjinKwon removed a comment on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047332517
There was a previous try to refer to (https://github.com/apache/spark/pull/31569). I agree that we should probably make it go away anyway .. cc @sarutak FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sarutak commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
sarutak commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047376702
Yeah, I thought it's better to replace them but there were some objections to do it in the previous PR. So, I just suggest another possible solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on a change in pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35560:
URL: https://github.com/apache/spark/pull/35560#discussion_r809998542
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -906,10 +906,10 @@ private[spark] class SparkSubmit extends Logging {
if (args.verbose) {
logInfo(s"Main class:\n$childMainClass")
- logInfo(s"Arguments:\n${childArgs.mkString("\n")}")
+ logInfo(s"Arguments:\n${childArgs.sorted.mkString("\n")}")
Review comment:
This is because I've created the branch for this PR from the branch of #35556. I will remove this commit from here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047508581
I wasn't aware of #31569. There some people said that it might be better to migrate to `s"abc"` instead of `Symbol("abc")`, and I agree.
I am fine with any of the following:
* close this PR as "Won't do"
* finish it
* re-work it to `s""`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047862027
Does `s"..."` do anything here? I thought that just expressed interpolated strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058514200
Right, but what else we use Symbol except representing Column? I might be missing something, but from what I've seen from test codes, majority of usage has been the same for $"...".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058491957
Is that the same thing? $".." is shorthand for a Column, not Scala Symbol
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1046931443
Is this all the occurences in test? this could be an OK unit to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sarutak commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
sarutak commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047349010
For making it easy to upgrade to Scala 3, another option would be to import `language.deprecated.symbolLiterals._` when we support Scala 3.
What do you think, all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1044620790
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1044535505
Heh, no right answer, but I'd say fix < 200 files at a time? maybe that gets to all test code? that would be a fine scope
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] jiangxb1987 commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
jiangxb1987 commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1065476806
Do we still have any tests for `'`? Since this is still supported, we should have at least one test to make sure it's working and if it's broken by a future Scala version, we can notice it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058523917
Follow-up fix sounds good to me. Easier to work based on this since we now just need to try finding `Symbol(` and replace there. Finding shorthand of Symbol `'` would be much more bothering work despite warning message will guide us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35560:
URL: https://github.com/apache/spark/pull/35560#discussion_r809969379
##########
File path: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala
##########
@@ -906,10 +906,10 @@ private[spark] class SparkSubmit extends Logging {
if (args.verbose) {
logInfo(s"Main class:\n$childMainClass")
- logInfo(s"Arguments:\n${childArgs.mkString("\n")}")
+ logInfo(s"Arguments:\n${childArgs.sorted.mkString("\n")}")
Review comment:
Was this an intended change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1048163361
Sorry! Muscle memory ... I meant `$"abc"` - https://github.com/wangyum/spark/commit/b428acb1322f534ad6f028c23286cf2ce18277f6
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058480954
Just to see our preference, given `$"..."` is not mentioned at all for deprecation and it is simpler than `Symbol("...")`, why not consistently using `$"..."`? My intuition is that `'` would be used as it is simplest representation to refer a column, and `$"..."` is going to be simplest once we are enforced to use `Symbol("...")` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058097677
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047370815
Does that just suppress the warning? I think just consistently using Symbol() is easier IMHO; there may not be that much left to change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35560: [SPARK-TODO][TESTS] Dont use deprecate symbol api
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1047334166
There was a previous try to refer to (https://github.com/apache/spark/pull/31569) cc @sarutak FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1054636646
Yeah don't bother with scalafmt here, just fix the long lines if there are only a few instances
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #35560:
URL: https://github.com/apache/spark/pull/35560
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1076406327
@martin-g any chance you have some time to take another look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1077667941
I promise to do it real soon! I have something else to finish first!
The new PR will be just for `master` branch, i.e. `3.4.x`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1058899649
Agreed! I will work on this soon!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] martin-g commented on pull request #35560: [SPARK-38351][TESTS] Don't use deprecate symbol API in test classes
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #35560:
URL: https://github.com/apache/spark/pull/35560#issuecomment-1066446565
@jiangxb1987 Yes, there are still usages of `'` in the tests. This PR changed just part of the test files to keep the diff not too big,
I didn't have time for the follow-up in the last two weeks but I will do it soon!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org