You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/05/16 09:22:39 UTC
[GitHub] [spark] panbingkun opened a new pull request, #41184: [MINOR][CONNECT] fix import order
panbingkun opened a new pull request, #41184:
URL: https://github.com/apache/spark/pull/41184
### What changes were proposed in this pull request?
The pr aims to fix import order for `connect` module.
### Why are the changes needed?
Make code style consistent.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA.
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1558665695
Oh, I think you can give it a try
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1558624517
In fact, Spark does not perform checks on the import order of Java 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] dongjoon-hyun commented on a diff in pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41184:
URL: https://github.com/apache/spark/pull/41184#discussion_r1196178581
##########
scalastyle-config.xml:
##########
@@ -295,7 +295,7 @@ This file is divided into 3 sections:
<parameter name="groups">java,scala,3rdParty,spark</parameter>
<parameter name="group.java">javax?\..*</parameter>
<parameter name="group.scala">scala\..*</parameter>
- <parameter name="group.3rdParty">(?!org\.apache\.spark\.).*</parameter>
+ <parameter name="group.3rdParty">(?!(javax?\.|scala\.|org\.apache\.spark\.)).*</parameter>
Review Comment:
Thank you for fixing 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] panbingkun commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1551123626
I hope to be qualified to do this one day in the future. Let me continue to strive for it! Congratulations again to @LuciferYang! haha
--
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 #41184: [MINOR][CONNECT] fix import order
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1549396581
Can we fix https://github.com/apache/spark/blob/master/scalastyle-config.xml to enforce 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] panbingkun commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1550941249
Through the debug plugin scalastyle, I found that the logic of the ImportOrderChecker rule is as follows, for example, in the xxx file, the following import：
<img width="541" alt="image" src="https://github.com/apache/spark/assets/15246973/1ba23a98-9f88-4b67-a41a-0862d590eaf0">
Firstly, when it iterates through the first import - `import ammonite.compiler.CodeClassWrapper`, it believes that there are no imports related to group `group.java` and `group.scala` at this time. The logic enters the check of the third group (`group.3rdParty`), and the rule we configured at this time only does not allow starting with `org.apache.spark.`. This condition is met, but it is not what we want.
By adjusting the regular matching rules of `group.3rdParty` group, the above situation can be avoided. We do not allow starting with `javax?.`, `scala.`, or `org.apache.spark.`.
The logic of `org.scalastyle.scalariform.ImportOrderChecker` is as follows:
<img width="588" alt="image" src="https://github.com/apache/spark/assets/15246973/0b47f217-19c0-4288-8537-c266731ec868">
https://github.com/scalastyle/scalastyle/blob/ec14399543d2d5ccf93c3713aa5df21793844791/src/main/scala/org/scalastyle/scalariform/ImportsChecker.scala#L238-L276
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1551130139
> I hope to be qualified to do this one day in the future. Let me continue to strive for it! Congratulations again to @LuciferYang! haha
Thanks @panbingkun :)
--
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] panbingkun commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1558650701
> If we could define a rule that checks the import group without checking the order within the group, it might be easier to accept
CheckStyle supports this feature
<img width="1141" alt="image" src="https://github.com/apache/spark/assets/15246973/3b2b0830-13fd-449b-ad87-1e5352d7117d">
--
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] panbingkun commented on pull request #41184: [MINOR][CONNECT] fix import order
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1549313503
I have checked all the codes of the `connect` 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] panbingkun commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1550945373
friendly ping @HyukjinKwon @dongjoon-hyun @srowen @LuciferYang
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1558630916
@panbingkun Checking the import order of Java code involves a lot of code changes, which may cause conflicts with previous versions. I think this is not worth because it is just `import order`.
If we could define a rule that checks the import group without checking the order within the group, it might be easier to accept
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1551116687
> @LuciferYang would you mind trying to merge this into `master` branch by `./dev/merge_spark_pr.py` when the tests pass? It would require you to set several environment variables like `JIRA_USERNAME`, and set the `remote` (please also read `./dev/merge_spark_pr.py` script). You should also install `pip install jira` (https://pypi.org/project/jira/) before running that script.
>
> For example, this is my remote:
>
> ```
> git remote -v
> ...
> apache https://github.com/apache/spark.git (fetch)
> apache https://github.com/apache/spark.git (push)
> apache-github https://github.com/apache/spark.git (fetch)
> apache-github https://github.com/apache/spark.git (push)
> ...
> origin https://github.com/HyukjinKwon/spark.git (fetch)
> origin https://github.com/HyukjinKwon/spark.git (push)
> ...
> upstream https://github.com/apache/spark.git (fetch)
> upstream https://github.com/apache/spark.git (push)
> ...
> ```
OK
--
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 #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1551107058
@LuciferYang would you mind trying to merge this into `master` branch by `./dev/merge_spark_pr.py`? It would require you to set several environment variables like `JIRA_USERNAME`, and `remote`.
For example, this is my remote:
```
git remote -v
...
apache https://github.com/apache/spark.git (fetch)
apache https://github.com/apache/spark.git (push)
apache-github https://github.com/apache/spark.git (fetch)
apache-github https://github.com/apache/spark.git (push)
...
```
--
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] LuciferYang closed pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
URL: https://github.com/apache/spark/pull/41184
--
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] LuciferYang commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1551415754
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] panbingkun commented on pull request #41184: [SPARK-43535][BUILD] Adjust the ImportOrderChecker rule to resolve long-standing import order issues
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41184:
URL: https://github.com/apache/spark/pull/41184#issuecomment-1558618352
@HyukjinKwon @dongjoon-hyun @LuciferYang
I have found issues similar to the above in the Java code. Do we need to make restrictions on this? Because the checkStyle plugin uses a similar configuration.
https://checkstyle.sourceforge.io/config_imports.html#ImportOrder
https://github.com/apache/spark/blob/eeab2e701330f7bc24e9b09ce48925c2c3265aa8/common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java#L18-L36
--
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