ssimeonov commented on a change in pull request #35084:
URL: https://github.com/apache/spark/pull/35084#discussion_r777164166
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -807,7 +807,7 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
extends Product with Tre
truncatedString(seq.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
case set: Set[_] =>
// Sort elements for deterministic behaviours
- val sortedSeq = set.toSeq.map(formatArg(_, maxFields).sorted)
+ val sortedSeq = set.toSeq.map(formatArg(_, maxFields)).sorted
Review comment:
Yeah, the implementation has a lot of duplicated code...
Would you be OK with applying the fix to `formatArg` and then DRYing
`argString` as follows?
**Before:**
```scala
case seq: Seq[_] =>
truncatedString(seq.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
case set: Set[_] =>
// Sort elements for deterministic behaviours
val sortedSeq = set.toSeq.map(formatArg(_, maxFields)).sorted
truncatedString(sortedSeq, "{", ", ", "}", maxFields) :: Nil
case array: Array[_] if array.isEmpty => Nil
case array: Array[_] =>
truncatedString(array.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
```
**After:**
```scala
case array: Array[_] if array.isEmpty => Nil
case xs @ (_: Seq[_] | _: Set[_] | Array[_]) =>
formatArg(xs, maxFields) :: Nil
```
It's a bigger change than the minimum required, but it'll clean thing up a
bit.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala
##########
@@ -807,7 +807,7 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
extends Product with Tre
truncatedString(seq.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
case set: Set[_] =>
// Sort elements for deterministic behaviours
- val sortedSeq = set.toSeq.map(formatArg(_, maxFields).sorted)
+ val sortedSeq = set.toSeq.map(formatArg(_, maxFields)).sorted
Review comment:
Yeah, the implementation has a lot of duplicated code...
Would you be OK with applying the fix to `formatArg` and then DRYing
`argString` as follows?
**Before:**
```scala
case seq: Seq[_] =>
truncatedString(seq.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
case set: Set[_] =>
// Sort elements for deterministic behaviours
val sortedSeq = set.toSeq.map(formatArg(_, maxFields)).sorted
truncatedString(sortedSeq, "{", ", ", "}", maxFields) :: Nil
case array: Array[_] if array.isEmpty => Nil
case array: Array[_] =>
truncatedString(array.map(formatArg(_, maxFields)), "[", ", ", "]",
maxFields) :: Nil
```
**After:**
```scala
case array: Array[_] if array.isEmpty => Nil
case xs @ (_: Seq[_] | _: Set[_] | Array[_]) =>
formatArg(xs, maxFields) :: Nil
```
It's a bigger change than the minimum required, but it'll clean things up a
bit.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
