pan3793 commented on code in PR #60078:
URL: https://github.com/apache/spark/pull/60078#discussion_r4477171122
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/InternalRowComparableWrapper.scala:
##########
@@ -114,6 +125,43 @@ object InternalRowComparableWrapper {
getInternalRowComparableWrapperFactory(partitionExpression.map(_.dataType))(partitionRow)
}
+ /**
+ * Renders a value the way a plan string shows it: by the value's type at
every level. Atomic
+ * values go through the [[Literal]] constructor, whose `toString` formats
dates, timestamps,
+ * decimals and binary; complex values are walked by type, because their own
`toString` shows
+ * internal encodings, hex words or an object identity. The constructor is
used rather than
+ * `Literal.create`, which runs the char/varchar write-side check and can
reject a stored value
+ * while a plan is being printed.
+ */
+ private def render(value: Any, dataType: DataType): String = {
+ if (value == null) {
+ "null"
+ } else {
+ dataType match {
+ case s: StructType =>
+ val r = value.asInstanceOf[InternalRow]
+ s.fields.iterator.zipWithIndex
+ .map { case (field, i) => render(r.get(i, field.dataType),
field.dataType) }
+ .mkString("[", ",", "]")
+ case ArrayType(elementType, _) =>
+ val a = value.asInstanceOf[ArrayData]
+ (0 until a.numElements()).map(i => render(a.get(i, elementType),
elementType))
+ .mkString("[", ",", "]")
+ case MapType(keyType, valueType, _) =>
+ val m = value.asInstanceOf[MapData]
+ (0 until m.numElements()).map { i =>
+ s"${render(m.keyArray().get(i, keyType), keyType)} -> " +
+ render(m.valueArray().get(i, valueType), valueType)
+ }.mkString("{", ",", "}")
+ case udt: UserDefinedType[_] => render(value, udt.sqlType)
+ case _: GeometryType | _: GeographyType =>
Review Comment:
Nit: this branch can't be reached from a `KeyedPartitioning`. Key layout
needs an ordering, and `OrderUtils.isOrderable` rejects `GeometryType` and
`GeographyType` (and `MapType`). The test only reaches it because the wrapper
builds its ordering lazily. Dropping this branch also removes the `HexFormat`
and `BinaryView` imports.
##########
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. The keys are
+ * summarized once they exceed the fields that `maxFields` leaves after the
expressions and the
+ * trailing fields, which are always printed. `dataTypes` is left out
because its naming is
Review Comment:
Nit: "which are always printed" doesn't hold. Below the fields around the
keys, the budget cuts them too. The new test asserts `simpleString(2) ==
"keyedpartitioning(x, true, ... 1 more fields)"`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/DistributionSuite.scala:
##########
@@ -17,8 +17,9 @@
package org.apache.spark.sql.catalyst
-import org.apache.spark.SparkFunSuite
+import org.apache.spark.{SparkConf, SparkFunSuite}
/* Implicit conversions */
Review Comment:
Nit: `/* Implicit conversions */` now sits above the `JavaSerializer` import
instead of the `dsl.expressions._` import it describes. Could `JavaSerializer`
go after it?
--
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]
