Copilot commented on code in PR #1872:
URL: https://github.com/apache/auron/pull/1872#discussion_r2721988807
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala:
##########
@@ -1001,12 +1001,12 @@ object NativeConverters extends Logging {
val children = e.children.map(Cast(_, e.dataType))
buildScalarFunction(pb.ScalarFunction.Coalesce, children, e.dataType)
- case e @ StringLPad(str, len, pad) =>
+ case _ @StringLPad(str, len, pad) =>
buildScalarFunction(
pb.ScalarFunction.Lpad,
Seq(str, castIfNecessary(len, LongType), pad),
StringType)
- case e @ StringRPad(str, len, pad) =>
+ case _ @StringRPad(str, len, pad) =>
buildScalarFunction(
pb.ScalarFunction.Rpad,
Review Comment:
This pattern alias (`_ @ ...`) is unnecessary and makes the match harder to
read. Since the matched value isn’t used, prefer matching directly on
`StringRPad(str, len, pad)` without an alias.
##########
spark-extension/src/main/scala/org/apache/spark/sql/execution/auron/plan/NativeShuffledHashJoinBase.scala:
##########
@@ -82,6 +82,8 @@ abstract class NativeShuffledHashJoinBase(
private def nativeBuildSide = buildSide match {
case JoinBuildLeft => pb.JoinSide.LEFT_SIDE
case JoinBuildRight => pb.JoinSide.RIGHT_SIDE
+ case other =>
+ throw new IllegalArgumentException(s"Unknown Join buildSide: $other")
Review Comment:
`buildSide` is a sealed trait with only `JoinBuildLeft`/`JoinBuildRight`
(see
`spark-extension/src/main/scala/org/apache/spark/sql/auron/join/JoinBuildSides.scala`).
Adding a catch-all `case other` removes compile-time exhaustiveness checking
and can turn future new cases into runtime failures. Prefer keeping this match
exhaustive without a default case so adding a new `JoinBuildSide` forces a
compile update here.
```suggestion
```
##########
spark-extension/src/main/scala/org/apache/spark/sql/auron/NativeConverters.scala:
##########
@@ -1001,12 +1001,12 @@ object NativeConverters extends Logging {
val children = e.children.map(Cast(_, e.dataType))
buildScalarFunction(pb.ScalarFunction.Coalesce, children, e.dataType)
- case e @ StringLPad(str, len, pad) =>
+ case _ @StringLPad(str, len, pad) =>
buildScalarFunction(
pb.ScalarFunction.Lpad,
Review Comment:
This pattern alias (`_ @ ...`) is unnecessary and makes the match harder to
read. Since the matched value isn’t used, prefer matching directly on
`StringLPad(str, len, pad)` without an alias.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
