cloud-fan commented on code in PR #58584:
URL: https://github.com/apache/spark/pull/58584#discussion_r3962204053
##########
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:
**Blocking (P1):** `catalystReturnType` is transient, so this recomputes it
from the executor's current `SQLConf`; the new wrapper branches also reject an
already-resolved `CharType`/`VarcharType` when the caller has first-class types
disabled. A view created under the persisted standard-semantics flag can
therefore be analyzed as `CHAR(5)` but execute with STRING converters, losing
padding and length checks or passing a different writable class to the Hive
function. Please carry the analyzed result and intermediate types through
evaluator serialization and select conversion from those types rather than
`SQLConf.get`, with persisted-view coverage that analyzes and executes under
opposite settings.
**Recommended change:** Bind Hive conversion construction to the Catalyst
types resolved during analysis.
**Why this works:** Carry the resolved GenericUDF return type and UDAF
partial-result types through evaluator serialization, and make target-aware
wrappers select CHAR/VARCHAR behavior from their supplied DataType rather than
the execution-time SQLConf.
**Scope:** HiveInspectors conversion selection, transient Hive GenericUDF
and UDAF evaluator schemas, and focused cross-session persisted-view tests.
**Compatibility:** Legacy expressions analyzed as STRING remain on the
legacy representation, while expressions analyzed as CHAR/VARCHAR retain their
declared length, padding, and default-collation boundary regardless of the
caller session.
**Risks:** A serialized analyzed type must stay aligned with the Hive
ObjectInspector used after evaluator reconstruction. Changing partial-result
schema ownership must keep UDAF serialization and deserialization symmetric.
**Constraints:** Preserve the existing unsupported Hive length errors and
default-collation behavior. Do not derive a different Catalyst type from
mutable caller configuration after analysis.
**Success:** Persisted Hive UDF and UDAF expressions return values matching
their analyzed CHAR/VARCHAR schemas when created and queried under opposite
settings, while inverse legacy plans continue to use STRING conversion.
##########
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:
**Non-blocking (P2):** The changed guards use `charVarcharFirstClassTypes`,
which is also true when `preserveCharVarcharTypeInfo` is enabled and standard
semantics is false, but all SPARK-59277 integration tests set only standard
semantics. Please add a preserve-only `HiveUDFSuite` case that asserts both the
first-class result type and the CHAR padding/value, so a regression that
narrows these branches to standard semantics cannot pass.
##########
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:
**Non-blocking (P2):** This now routes every SerDe output value through `Any
=> Any` and `SpecificInternalRow.update`, so primitive fields no longer use the
existing `HiveStructField` unwrapper's direct `setBoolean`/`setInt`/`setLong`
paths on this per-row loop. Please add a DataType-aware in-place overload that
preserves the specialized primitive setters and uses target-type-aware generic
conversion only for CHAR/VARCHAR and other types that need it.
##########
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:
**Nit (P3):** The Scaladoc above this overload still says every
non-nanosecond target delegates to the `ObjectInspector`-only overload, but
these new CHAR/VARCHAR branches pad values and reject excess length. Please
update the contract to describe these target-type checks.
--
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]
