cloud-fan commented on code in PR #58584:
URL: https://github.com/apache/spark/pull/58584#discussion_r4008642285
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDAFSuite.scala:
##########
@@ -200,6 +201,25 @@ class HiveUDAFSuite extends QueryTest
}
}
+ test("SPARK-59277: Hive UDAF supports first-class CHAR/VARCHAR") {
Review Comment:
**Non-blocking (P2):** Could we add a UDAF fixture whose PARTIAL1 inspector
type differs from FINAL? `resolvedDataTypes` is an ordered `(partial, final)`
pair: `AggregationBufferSerDe` uses the first entry while result conversion
uses the second. `GenericUDAFMax` exposes the same type in both modes, so a
tuple swap or use of the final type for the shuffle buffer would still pass
this test. A distinct-type fixture would protect the serialization contract
added here.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveInspectorSuite.scala:
##########
@@ -292,6 +300,140 @@ class HiveInspectorSuite extends SparkFunSuite with
HiveInspectors {
assert(typeInfo2.scale() === 10)
}
+ test("SPARK-59277: Hive object inspectors preserve CHAR/VARCHAR type
information") {
+ withFirstClassCharVarchar(enabled = true) {
+ Seq[DataType](CharType(5), VarcharType(7)).foreach { dataType =>
+ val inspector =
toInspector(dataType).asInstanceOf[PrimitiveObjectInspector]
+ assert(inspectorToDataType(inspector) === dataType)
+ dataType match {
+ case c: CharType =>
+ assert(inspector.getTypeInfo.asInstanceOf[CharTypeInfo].getLength
=== c.length)
+ case v: VarcharType =>
+
assert(inspector.getTypeInfo.asInstanceOf[VarcharTypeInfo].getLength ===
v.length)
+ }
+ }
+ }
+ }
+
+ test("SPARK-59277: Hive object inspectors accept collated CHAR/VARCHAR
values") {
+ withFirstClassCharVarchar(enabled = true) {
+ Seq[DataType](
+ CharType(5, "UTF8_LCASE"),
+ VarcharType(7, "UNICODE_CI")).foreach { dataType =>
+ val inspector = toInspector(dataType)
+ val value = UTF8String.fromString(dataType match {
+ case _: CharType => "ab"
+ case _: VarcharType => "abc"
+ })
+ val expectedValue = dataType match {
+ case _: CharType => UTF8String.fromString("ab ")
+ case _: VarcharType => value
+ }
+ val expectedType = dataType match {
+ case c: CharType => CharType(c.length)
+ case v: VarcharType => VarcharType(v.length)
+ }
+ assert(inspectorToDataType(inspector) === expectedType)
+ assert(unwrap(wrap(value, inspector, dataType), inspector) ===
expectedValue)
+ }
+ }
+ }
+
+ test("SPARK-59277: Hive object inspectors support nested CHAR/VARCHAR") {
+ withFirstClassCharVarchar(enabled = true) {
+ val dataType = StructType(Seq(
+ StructField("chars", ArrayType(CharType(4))),
+ StructField("varchars", MapType(IntegerType, VarcharType(8)))))
+ val inspector = toInspector(dataType)
+ assert(inspectorToDataType(inspector) === dataType)
+
+ val input = InternalRow(
+ new GenericArrayData(Array[Any](UTF8String.fromString("a"))),
+ ArrayBasedMapData(
+ Array[Any](1),
+ Array[Any](UTF8String.fromString("value"))))
+ val result = unwrapperFor(inspector, dataType)(
+ wrap(input, inspector, dataType)).asInstanceOf[InternalRow]
+ assert(result.getArray(0).getUTF8String(0) === UTF8String.fromString("a
"))
+ assert(result.getMap(1).valueArray().getUTF8String(0) ===
UTF8String.fromString("value"))
+
+ val outerType = StructType(Seq(StructField("nested", dataType)))
+ val outerInspector =
toInspector(outerType).asInstanceOf[StructObjectInspector]
+ val field = outerInspector.getAllStructFieldRefs.get(0)
+ val targetRow = new SpecificInternalRow(Seq(dataType))
+ unwrapperFor(field, dataType)(wrap(input, inspector, dataType),
targetRow, 0)
+ val nestedResult = targetRow.getStruct(0, dataType.length)
+ assert(nestedResult.getArray(0).getUTF8String(0) ===
UTF8String.fromString("a "))
+ assert(
+ nestedResult.getMap(1).valueArray().getUTF8String(0) ===
UTF8String.fromString("value"))
+ }
+ }
+
+ test("SPARK-59277: Hive constant inspectors preserve CHAR/VARCHAR type
information") {
Review Comment:
**Non-blocking (P2):** Please also exercise null values here. Both new
constant-inspector helpers have explicit `value == null` branches, but this
loop only passes `"abc"`, and the older constant-null matrix does not include
`CharType` or `VarcharType`. Without a null case, removing either branch can
turn a valid NULL literal into a cast/construction failure without failing this
suite.
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:
##########
@@ -897,6 +953,19 @@ private[hive] trait HiveInspectors {
(value: Any, row: InternalRow, ordinal: Int) => row(ordinal) =
unwrapper(value)
}
+ /**
+ * Builds an in-place unwrapper that also honors target-type-specific
conversions.
Review Comment:
**Nit (P3):** Could we narrow this comment to say that the overload honors
CHAR/VARCHAR-specific conversion? The implementation only calls the
DataType-aware unwrapper when `hasCharVarchar(dataType)`; nanosecond timestamp
targets, for example, still fall through to `unwrapperFor(field)` even though
the adjacent overload treats them specially. As written, this promises a
broader contract than the method implements.
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFEvaluators.scala:
##########
@@ -162,9 +162,12 @@ class HiveGenericUDFEvaluator(
}
@transient
- private lazy val unwrapper: Any => Any = unwrapperFor(returnInspector)
+ private lazy val catalystReturnType = inspectorToDataType(returnInspector)
Review Comment:
Confirmed. The analyzed GenericUDF and UDAF types now survive copying and
evaluator reconstruction, and the persisted-view test covers both
opposite-configuration directions. Thanks for the fix.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3962204053","thread_id":"inline:3962204053","verdict_sha256":"0c5976318b24590c378a566e055ad8eeb77ad8abf531e89f57f1b5020c23e5c9"}
-->
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDFSuite.scala:
##########
@@ -892,6 +892,42 @@ class HiveUDFSuite extends QueryTest with
TestHiveSingleton {
hiveContext.reset()
}
+ test("SPARK-59277: Hive UDF and UDTF support first-class CHAR/VARCHAR") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
Review Comment:
Confirmed. The preserve-only case now disables standard semantics and
asserts both CharType(5) and the padded value. Thanks.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3962204063","thread_id":"inline:3962204063","verdict_sha256":"0c5976318b24590c378a566e055ad8eeb77ad8abf531e89f57f1b5020c23e5c9"}
-->
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:
##########
@@ -829,6 +864,26 @@ private[hive] trait HiveInspectors {
null
}
}
+ case (_, c: CharType) =>
Review Comment:
Confirmed. The typed unwrapper Scaladoc now documents the CHAR/VARCHAR
length and padding checks. Thanks.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3962204070","thread_id":"inline:3962204070","verdict_sha256":"0c5976318b24590c378a566e055ad8eeb77ad8abf531e89f57f1b5020c23e5c9"}
-->
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationExec.scala:
##########
@@ -130,7 +132,7 @@ private[hive] case class HiveScriptTransformationExec(
if (dataList.get(i) == null) {
mutableRow.setNullAt(i)
} else {
- unwrappers(i)(dataList.get(i), mutableRow, i)
+ mutableRow.update(i, unwrappers(i)(dataList.get(i)))
Review Comment:
Confirmed. Primitive fields retain their specialized setters, while
CHAR/VARCHAR-bearing fields use the target-aware path. Thanks.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3962204075","thread_id":"inline:3962204075","verdict_sha256":"0c5976318b24590c378a566e055ad8eeb77ad8abf531e89f57f1b5020c23e5c9"}
-->
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:
##########
@@ -897,6 +953,19 @@ private[hive] trait HiveInspectors {
(value: Any, row: InternalRow, ordinal: Int) => row(ordinal) =
unwrapper(value)
}
+ /**
+ * Builds an in-place unwrapper that also honors target-type-specific
conversions.
+ */
+ def unwrapperFor(
+ field: HiveStructField,
+ dataType: DataType): (Any, InternalRow, Int) => Unit = dataType match {
+ case _: CharType | _: VarcharType =>
+ val unwrapper = unwrapperFor(field.getFieldObjectInspector, dataType)
+ (value: Any, row: InternalRow, ordinal: Int) => row(ordinal) =
unwrapper(value)
+ case _ =>
Review Comment:
Confirmed. Nested CHAR/VARCHAR field types now select recursive target-aware
conversion, and the focused test exercises that in-place path. Thanks.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:3992124822","thread_id":"inline:3992124822","verdict_sha256":"0c5976318b24590c378a566e055ad8eeb77ad8abf531e89f57f1b5020c23e5c9"}
-->
--
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]
