srielau commented on code in PR #58584:
URL: https://github.com/apache/spark/pull/58584#discussion_r4064656814
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:
##########
@@ -1122,6 +1258,51 @@ private[hive] trait HiveInspectors {
case _: JavaVoidObjectInspector => NullType
}
+ /**
+ * Analysis snapshots the Catalyst return type, but runtime inspectors are
rebuilt from the
+ * current children (including foldability and session CHAR/VARCHAR
settings). STRING may drift
+ * to or from a bounded string type across that boundary. Two bounded types
must match exactly:
+ * accepting a different kind or length would apply the snapshotted
conversion to an incompatible
+ * runtime value.
+ */
+ def checkCompatibleHiveReturnType(
+ inspector: ObjectInspector,
+ expectedType: DataType): Unit = {
+ checkCompatibleHiveReturnType(
+ inspectorToDataType(inspector, preserveCharVarchar = true),
+ expectedType)
+ }
+
+ def checkCompatibleHiveReturnType(
+ runtimeType: DataType,
+ expectedType: DataType): Unit = {
+ if (!compatibleHiveReturnType(runtimeType, expectedType)) {
+ throw SparkException.internalError(
+ s"Hive function runtime type ${runtimeType.catalogString} is
incompatible " +
+ s"with analysis type ${expectedType.catalogString}.")
+ }
+ }
+
+ private def compatibleHiveReturnType(
+ runtimeType: DataType,
+ expectedType: DataType): Boolean = {
+ (runtimeType, expectedType) match {
+ case (rt: CharType, et: CharType) => rt == et
+ case (rt: VarcharType, et: VarcharType) => rt == et
+ case (_: CharType | _: VarcharType, _: CharType | _: VarcharType) =>
false
+ case (_: StringType, _: CharType | _: VarcharType) => true
+ case (_: CharType | _: VarcharType, _: StringType) => true
+ case (ArrayType(rt, _), ArrayType(et, _)) =>
compatibleHiveReturnType(rt, et)
+ case (MapType(rk, rv, _), MapType(ek, ev, _)) =>
+ compatibleHiveReturnType(rk, ek) && compatibleHiveReturnType(rv, ev)
+ case (rt: StructType, et: StructType) if rt.length == et.length =>
+ rt.fields.zip(et.fields).forall { case (rf, ef) =>
Review Comment:
Could we include the struct field name in this recursive compatibility
check? `unwrapperFor(StructObjectInspector, StructType)` pairs runtime
inspector fields with the snapshotted schema by ordinal, so runtime
`struct<b:int,a:int>` is currently accepted against analysis
`struct<a:int,b:int>` and its values are silently exposed under the wrong
names. Requiring corresponding names to match before recursing keeps the
snapshot guard from accepting renamed or reordered output.
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:
##########
@@ -897,6 +971,25 @@ private[hive] trait HiveInspectors {
(value: Any, row: InternalRow, ordinal: Int) => row(ordinal) =
unwrapper(value)
}
+ /**
+ * Builds an in-place unwrapper using the target Catalyst `dataType` only
when target-aware
+ * conversion is required. Other types retain the primitive setter fast
paths.
+ */
+ def unwrapperFor(
Review Comment:
After Script TRANSFORM support moved to the follow-up, this overload has no
production caller; the only remaining calls are in `HiveInspectorSuite`. Could
we remove it and the now-orphaned in-place/nanosecond test coverage from this
PR, and let the follow-up introduce it together with its production call?
--
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]
