You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/11/08 10:17:13 UTC
[PR] [WIP][SPARK-45825][SQL] Fix some scala compile warnings in package sql/catalyst [spark]
beliefer opened a new pull request, #43717:
URL: https://github.com/apache/spark/pull/43717
### What changes were proposed in this pull request?
Fix some scala compile warnings in package sql/catalyst. These scala compile warnings show below.
- Replace .size with .length on arrays and strings
- The enclosing block is redundant
- Replace with .head
- Replace with .nonEmpty
- Replace with .isDefined
- Unnecessary parentheses
- Replace with .isEmpty
### Why are the changes needed?
Improve the code and fix scala compile warnings
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
Exists test cases.
### Was this patch authored or co-authored using generative AI tooling?
'No'.
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43717:
URL: https://github.com/apache/spark/pull/43717#discussion_r1387462013
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -197,7 +197,7 @@ case class Mask(
*/
override def eval(input: InternalRow): Any = {
Mask.transformInput(
- children(0).eval(input),
+ children.head.eval(input),
Review Comment:
Considering the characteristics and best practices of scala, personally, I feel the `.head` is better than `(0)`.
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803109724
cc @HyukjinKwon
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1807258015
Just for a record, I'm going to be away from this PR because I'm neutral. Please feel free to proceed by ignoring my previous comments. Thanks.
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803227663
Do we have any `tools` that can help prohibit the `next time`? Eg: scalafmt ?
Otherwise, I'm worried that these problems will arise after some time.
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer closed pull request #43717: [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst
URL: https://github.com/apache/spark/pull/43717
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43717:
URL: https://github.com/apache/spark/pull/43717#discussion_r1387445107
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -197,7 +197,7 @@ case class Mask(
*/
override def eval(input: InternalRow): Any = {
Mask.transformInput(
- children(0).eval(input),
+ children.head.eval(input),
Review Comment:
You means the old one is better?
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803114073
> Are other modules to be completed in followup or will continue to update this one? @beliefer
I created an umbrella issue, please see https://issues.apache.org/jira/browse/SPARK-45823
--
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
Re: [PR] [WIP][SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1801806543
> I personally agree with some of the changes, but will it actually print the warning log during compilation?
No. These warning appeared when the IDE(scala plugin) compiling.
--
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
Re: [PR] [WIP][SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1801766488
I personally agree with this change, but will it actually print the warning log during compilation?
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1805247322
ping @HyukjinKwon Could you take a 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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1820873259
@beliefer I think the fix for `Replace .size with .length on arrays and strings` is worth it.
`Array` and `String` are both native objects of Java, and `length` is Java method. The 'size' function (defined in `ArrayOps` and `StringOps`) is a proxy for the `length` method. Directly using `length` can reduce the call stack, and I think we can fix all of this case in one pr.
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43717:
URL: https://github.com/apache/spark/pull/43717#discussion_r1387462013
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -197,7 +197,7 @@ case class Mask(
*/
override def eval(input: InternalRow): Any = {
Mask.transformInput(
- children(0).eval(input),
+ children.head.eval(input),
Review Comment:
Considering the best practices of scala, personally, I feel the `.head` is better than `(0)`.
--
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
Re: [PR] [SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43717:
URL: https://github.com/apache/spark/pull/43717#discussion_r1387438376
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/numberFormatExpressions.scala:
##########
@@ -242,7 +242,7 @@ object ToCharacterBuilder extends ExpressionBuilder {
override def build(funcName: String, expressions: Seq[Expression]): Expression = {
val numArgs = expressions.length
if (numArgs == 2) {
- val (inputExpr, format) = (expressions(0), expressions(1))
+ val (inputExpr, format) = (expressions.head, expressions(1))
Review Comment:
I'm not sure about this part.
--
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
Re: [PR] [SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803101246
Are other modules to be completed in followup or will continue to update this one? @beliefer
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1820957029
@LuciferYang I see. Let me create it later.
--
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
Re: [PR] [WIP][SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1801810530
> > I personally agree with some of the changes, but will it actually print the warning log during compilation?
>
> No. These warning appeared when the IDE(scala plugin) compiling.
Got
--
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
Re: [PR] [SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43717:
URL: https://github.com/apache/spark/pull/43717#discussion_r1387438134
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -197,7 +197,7 @@ case class Mask(
*/
override def eval(input: InternalRow): Any = {
Mask.transformInput(
- children(0).eval(input),
+ children.head.eval(input),
Review Comment:
In this case, the old one is more consistent to the next lines.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/maskExpressions.scala:
##########
@@ -237,7 +237,7 @@ case class Mask(
ctx: CodegenContext,
ev: ExprCode,
f: (String, String, String, String, String) => String): ExprCode = {
- val firstGen = children(0).genCode(ctx)
+ val firstGen = children.head.genCode(ctx)
Review Comment:
ditto.
--
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
Re: [PR] [SPARK-45825][SQL] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803097528
ping @dongjoon-hyun @LuciferYang cc @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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803258795
> > Otherwise, I'm worried that these problems will arise after some time.
>
> Good question. But I feel the `scalafmt` can't resolve these issue. @LuciferYang Do you know how to add the check rules of IntelliJ to the Spark builder?
These are two inspection systems, at least now I don't know how to do it We need some additional investigation
--
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
Re: [PR] [SPARK-45825][SQL][CORE][GRAPHX] Fix some scala compile warnings in module sql/catalyst [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #43717:
URL: https://github.com/apache/spark/pull/43717#issuecomment-1803250355
> Otherwise, I'm worried that these problems will arise after some time.
Good question. But I feel the `scalafmt` can't resolve these issue.
@LuciferYang Do you know how to add the check rules of IntelliJ to the Spark builder?
--
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