ulysses-you commented on code in PR #60078:
URL: https://github.com/apache/spark/pull/60078#discussion_r4465666098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -39,6 +39,10 @@ class InternalRowComparableWrapper private (
@transient private var _structType: StructType,
@transient private var _ordering: BaseOrdering) extends Serializable {
+ require(row.numFields == dataTypes.length,
+ s"The row has ${row.numFields} fields, but the types it is compared at
have " +
+ s"${dataTypes.length}")
Review Comment:
Removed in 95b3517f907. It was a new behavior change (a wider row used to
work) and it ran once per record in the shuffle key extractor, so it is gone
rather than moved.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -39,6 +39,10 @@ class InternalRowComparableWrapper private (
@transient private var _structType: StructType,
@transient private var _ordering: BaseOrdering) extends Serializable {
+ require(row.numFields == dataTypes.length,
Review Comment:
With the require removed this path is unchanged: a narrower row still fails
in the sort before a wrapper is built. Covering both directions would need a
width check at the scan boundary, as a separate change.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -39,6 +39,10 @@ class InternalRowComparableWrapper private (
@transient private var _structType: StructType,
@transient private var _ordering: BaseOrdering) extends Serializable {
+ require(row.numFields == dataTypes.length,
Review Comment:
Removed with the require in 95b3517f907.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -65,6 +69,9 @@ class InternalRowComparableWrapper private (
_ordering
}
+ override def toString: String =
+ row.toSeq(dataTypes).mkString("[", ",", "]")
Review Comment:
Done: each value goes through Literal.create(value, dataType).toString.
Tests cover DATE, TIMESTAMP, TIMESTAMP_NTZ, TIME, DECIMAL, BINARY, CHAR and
VARCHAR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -65,6 +69,9 @@ class InternalRowComparableWrapper private (
_ordering
}
+ override def toString: String =
Review Comment:
Noted in the toString scaladoc: values print as-is, so keys that compare
equal under UTF8_LCASE can still print differently.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -750,15 +750,30 @@ case class KeyedPartitioning(
/**
* Prints the layout's contents where the value object would print, which
keeps the plan string as
* it was before the layout held them. The list is curated rather than the
layout's own fields,
- * for two reasons. `partitionKeys` has to be printed as a `Seq` for
`maxFields` to truncate it,
- * and a partitioning can hold one key per split. And `dataTypes` has its
naming erased, so
- * printing it would put a struct field name into a plan that appears
nowhere in the query. A
- * field the layout grows is therefore a decision here, and the default is
to print it: the plan
- * string is the only place a reader sees why a marked partitioning still
shuffles.
+ * for two reasons. A partitioning can hold one key per split, so
`partitionKeys` is summarized:
+ * as many keys as `spark.sql.debug.maxToStringFields` has room for, then
their total. And
+ * `dataTypes` has its naming erased, so printing it would put a struct
field name into a plan
+ * that appears nowhere in the query. A field the layout grows is therefore
a decision here, and
+ * the default is to print it: the plan string is the only place a reader
sees why a marked
+ * partitioning still shuffles.
*/
- override protected def stringArgs: Iterator[Any] =
- Iterator(expressions, partitionKeys, isGrouped, isCollapsed) ++
unknownKeysOrigin ++
- ungroupingOrigin
+ override protected def stringArgs: Iterator[Any] = {
+ // `Expression.toString` truncates the flattened argument list at
`maxToStringFields`. The
+ // expressions and the fields printed after the keys keep their slots, and
the keys take what
+ // is left; when they cannot all fit, their total takes one key slot. When
there is not even a
+ // slot left for that, the total still wins over a trailing field, which
the outer truncation
+ // then drops.
+ val trailing = Seq[Any](isGrouped, isCollapsed) ++ unknownKeysOrigin ++
ungroupingOrigin
+ val keyBudget = math.max(0, SQLConf.get.maxToStringFields -
expressions.size - trailing.size)
+ val keys: Seq[Any] =
+ if (partitionKeys.length <= keyBudget) {
+ partitionKeys
+ } else {
+ partitionKeys.take(math.max(0, keyBudget - 1)) :+
Review Comment:
Reworked: maxFields is a hard cap and both levels use truncatedString. At
keyBudget == 0 the key summary is dropped; below it the trailing fields are cut
too, so the double ellipsis cannot occur.
--
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]
