ulysses-you commented on code in PR #60078:
URL: https://github.com/apache/spark/pull/60078#discussion_r4475702079
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -65,6 +66,11 @@ class InternalRowComparableWrapper private (
_ordering
}
+ override def toString: String =
+ row.toSeq(dataTypes).zip(dataTypes)
+ .map { case (value, dataType) => Literal.create(value,
dataType).toString }
Review Comment:
Fixed in baa3342eb54. Atomic values now go through the Literal(value,
dataType) constructor, not Literal.create, so no char/varchar write-side
conversion runs. Tests cover CHAR/VARCHAR keys under both first-class settings,
including an over-length VARCHAR and an unpadded shorter CHAR.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -748,17 +748,32 @@ case class KeyedPartitioning(
override def dataType: DataType = IntegerType
/**
- * 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.
+ * Prints the layout as the partition keys followed by the flags and
origins, with the keys
+ * summarized when there are more than `maxFields` of them. `dataTypes` is
left out because its
+ * naming is erased, so printing it would put a struct field name into a
plan that appears
+ * nowhere in the query. Every other field of the layout is printed: the
plan string is the only
+ * place a reader sees why a marked partitioning still shuffles. Keys print
as values, so two
+ * keys that `equals` considers equal can print differently, e.g. `'a'` and
`'A'` under
+ * `UTF8_LCASE`.
*/
- override protected def stringArgs: Iterator[Any] =
- Iterator(expressions, partitionKeys, isGrouped, isCollapsed) ++
unknownKeysOrigin ++
- ungroupingOrigin
+ override def toString: String = simpleString(SQLConf.get.maxToStringFields)
+
+ override def simpleString(maxFields: Int): String = {
+ val trailing = Seq[Any](isGrouped, isCollapsed) ++ unknownKeysOrigin ++
ungroupingOrigin
+ // The keys take the fields that the expressions and the trailing flags
leave.
+ val keyBudget = maxFields - expressions.size - trailing.size
+ val keys = if (keyBudget < 1) {
Review Comment:
Kept as is. Dropping the summary when the surrounding fields fill the budget
is what keeps the string to one ellipsis, and the scaladoc now says the keys
are summarized against the fields left after the expressions and the trailing
fields rather than past maxFields.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/physical/partitioning.scala:
##########
@@ -748,17 +748,32 @@ case class KeyedPartitioning(
override def dataType: DataType = IntegerType
/**
- * 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.
+ * Prints the layout as the partition keys followed by the flags and
origins, with the keys
+ * summarized when there are more than `maxFields` of them. `dataTypes` is
left out because its
Review Comment:
Rewritten in baa3342eb54: the keys are summarized once they exceed the
fields that maxFields leaves after the expressions and the trailing fields.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapperSuite.scala:
##########
@@ -153,4 +155,42 @@ class InternalRowComparableWrapperSuite extends
SparkFunSuite {
assert(wrap(IntegerType, InternalRow(1)) !== wrap(LongType,
InternalRow(1L)),
"and a type that decides where a value belongs still tells them apart")
}
+
+ test("SPARK-60859: a wrapper prints its row values") {
+ val factory = InternalRowComparableWrapper
+ .getInternalRowComparableWrapperFactory(Seq(IntegerType, StringType))
+ assert(factory(InternalRow(1, UTF8String.fromString("a"))).toString ===
"[1,a]")
+ assert(factory(InternalRow(null, UTF8String.fromString("a"))).toString ===
"[null,a]")
+
+ // UnsafeRow's own toString is a hex dump of its words, so the wrapper has
to read the values.
+ val unsafeProjection =
UnsafeProjection.create(Array[DataType](IntegerType, StringType))
+ val unsafeRow = unsafeProjection(InternalRow(2,
UTF8String.fromString("b")))
+ assert(factory(unsafeRow).toString === "[2,b]")
+ }
+
+ test("SPARK-60859: a wrapper prints key types readably") {
+ // Date, Timestamp in UTC, and a binary key.
+ val common =
InternalRowComparableWrapper.getInternalRowComparableWrapperFactory(
+ Seq(DateType, TimestampType, BinaryType))
+ val commonRow = InternalRow(19724, 1704164645000000L, Array[Byte](1, 2))
+ withSQLConf(SQLConf.SESSION_LOCAL_TIMEZONE.key -> "UTC") {
+ assert(common(commonRow).toString === "[2024-01-02,2024-01-02
03:04:05,0x0102]")
+ }
+
+ // Decimal, TimestampNTZ, Time in nanoseconds, Char and Varchar.
+ val other =
InternalRowComparableWrapper.getInternalRowComparableWrapperFactory(
+ Seq(DecimalType(10, 2), TimestampNTZType, TimeType(), CharType(5),
VarcharType(5)))
+ val otherRow = InternalRow(
+ Decimal(BigDecimal("12.34"), 10, 2),
+ 1704164645000000L, // 2024-01-02 03:04:05
+ 11045000000000L, // 03:04:05 in nanoseconds since midnight
+ UTF8String.fromString("ab"),
+ UTF8String.fromString("cd"))
+ assert(other(otherRow).toString === "[12.34,2024-01-02
03:04:05,03:04:05,ab,cd]")
+
+ // Two collated keys that compare equal still print their own case.
+ val collated = InternalRowComparableWrapper
+ .getInternalRowComparableWrapperFactory(Seq(StringType("UTF8_LCASE")))
+ assert(collated(InternalRow(UTF8String.fromString("A"))).toString ===
"[A]")
Review Comment:
Fixed in baa3342eb54: the test builds both 'a' and 'A', asserts they compare
equal, and asserts each prints its own case.
--
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]
