Conversation
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The executor-side conversion state is the blocking issue. Because transient Hive return and intermediate types are rebuilt from the querying session's SQLConf, a persisted view can advertise CHAR/VARCHAR while executing with STRING converters, changing values or Hive writable classes across callers. Bind conversion to the analyzed types and add an opposite-configuration persisted-view regression before merging.
The review also found three non-blocking follow-ups: cover the preserve-only first-class mode, retain primitive-specific setters in the script-output hot path, and correct the typed-unwrapper Scaladoc.
Findings
4 total: 0 P0, 1 P1, 2 P2, 1 P3.
Blocking (P1)
- Bind Hive conversion to the analyzed CHAR/VARCHAR type —
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFEvaluators.scala:165— see inline.
Non-blocking (P2)
- Exercise the preserve-only first-class mode —
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDFSuite.scala:896— see inline. - Preserve primitive setters for Hive script output —
sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationExec.scala:135— see inline.
Nit (P3)
- Describe the CHAR/VARCHAR checks in the typed unwrapper contract —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:867— see inline.
Verification
- The standard-semantics configuration is PERSISTED specifically so a view keeps its resolved CHAR/VARCHAR behavior across caller sessions.
- Preserve-only mode independently activates the same changed conversion guards but is absent from the new Hive integration tests.
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
I found two non-blocking correctness gaps in the newly supported paths. Nested CHAR/VARCHAR values from a Hive SerDe can bypass target-aware conversion, and a bound Hive UDTF can lose its analyzed element schema across a persisted-view configuration change. The scalar and aggregate fixes, legacy fallback, preserve-only coverage, and prior review follow-ups otherwise look consistent.
Findings
2 total: 0 P0, 0 P1, 2 P2, 0 P3.
Non-blocking (P2)
- Apply target-aware conversion to nested SerDe CHAR/VARCHAR —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:965— see inline. - Preserve the analyzed UDTF element schema across copies —
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala:253— see inline.
Re-review status
Prior AI findings: 4 addressed, 0 still present; additional unresolved findings in this review: 2.
New attribution: 1 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The two findings from the previous review are resolved: nested SerDe fields now select recursive target-aware conversion, and HiveGenericUDTF preserves its analyzed element schema across child replacement and opposite-session execution. The four earlier threads are also addressed in the current source.
This round found no blocking production defect. The remaining items are two focused test blind spots in newly introduced branches/contracts—null CHAR/VARCHAR constants and UDAFs with distinct partial/final types—plus a narrow documentation overstatement in the new in-place unwrapper overload.
Findings
3 total: 0 P0, 0 P1, 2 P2, 1 P3.
Non-blocking (P2)
- Exercise distinct UDAF partial and final types —
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDAFSuite.scala:204— see inline. - Cover null CHAR/VARCHAR constants —
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveInspectorSuite.scala:372— see inline.
Nit (P3)
- Narrow the in-place unwrapper contract to CHAR/VARCHAR —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:957— see inline.
Re-review status
Prior AI findings: 2 addressed, 0 still present; additional unresolved findings in this review: 3.
New attribution: 0 newly introduced, 3 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
Serialize a concrete Catalyst type for runtime evaluation so executor Hive UDF conversion does not re-infer CHAR/VARCHAR from SQLConf.
…ORM overflow Store inferred Hive UDF/UDTF/UDAF result types as required case-class fields so Java serialization cannot drop them, and deserialize Hive TRANSFORM CHAR/VARCHAR as STRING so first-class length checks raise EXCEED_LIMIT_LENGTH instead of truncating.
Share the unbounded STRING rewrite so first-class CHAR/VARCHAR overflow checks still apply, keep malformed nested JSON as null, and tighten Hive return-type compatibility to STRING boundary drift only.
transformRecursively stops at the first matching MapType, so nested integer keys stayed INT while Jackson produced strings.
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The three findings from the previous review are resolved in the current source: typed script conversion now matches its documentation, the UDAF fixture distinguishes partial STRING from final CHAR across shuffle, and null CHAR/VARCHAR constant inspectors are covered.
This round found one contained production defect in the no-SerDe nested script-output path: direct whole-map Cast evaluation can materialize null or duplicate keys. It also found two focused Hive inspector test gaps and two small documentation defects. None is blocking, but all five are actionable before merge.
Findings
5 total: 0 P0, 0 P1, 3 P2, 2 P3.
Non-blocking (P2)
- Cover map and struct return-type compatibility —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:1282— see inline. - Validate restored JSON map keys before building MapData —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:275— see inline. - Exercise the VARCHAR lower-bound rejection —
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveInspectorSuite.scala:524— see inline.
Nit (P3)
- Use the lifecycle noun in this Scaladoc —
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFEvaluators.scala:117— see inline. - Spell LazySimpleSerDe with its exact class name —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:215— see inline.
Re-review status
Prior AI findings: 3 addressed, 0 still present; additional unresolved findings in this review: 5.
New attribution: 4 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 5 prior findings addressed, 0 remaining; 2 findings in this pass (1 newly introduced, 1 late catch).
The CHAR/VARCHAR configuration paths and Hive UDF, UDTF, UDAF, SerDe, and no-SerDe conversion layers are otherwise consistent. The two actionable findings are inline.
Verification: added-comment and @link/@see scans found no other issues; git diff --check passes.
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 2 prior findings addressed, 0 remaining; 4 findings in this pass (3 newly introduced, 1 late catch).
The fresh-builder production fix is sound: each restored map now gets an independent ArrayBasedMapBuilder, so a failed row cannot poison the next row. The remaining findings are inline.
Verification: focused SparkScriptTransformationSuite -- -z SPARK-59277 passed 8/8; Scalastyle and git diff --check passed.
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 4 prior findings addressed, 0 remaining; 1 new late catch.
The same-partition regression, structural restorer invariant, and both wording corrections are resolved in d9044026aa2. The production and test changes otherwise look clean.
One PR-description correction remains: the test-count paragraph still says 30 uniquely named tests, 6 inherited no-SerDe cases, and 36 total executions. The current source has 32 uniquely named SPARK-59277 tests; HiveScriptTransformationSuite inherits 8 no-SerDe cases, so the full filtered run executes 40 tests. The latest focused SparkScriptTransformationSuite run at d9044026aa2 passed 8/8.
Verification: focused suite 8/8, Scalastyle, and git diff --check passed.
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The five findings from the previous review are addressed in the current source. This round found two correctness gaps where bounded map-key normalization can create duplicate-key MapData, one per-row conversion cost in the no-SerDe nested path, and three focused negative-path test gaps for null-token and runtime-inspector compatibility guards. This crosses several hard boundaries at once: Catalyst containers, Hive object inspectors, runtime inspector reconstruction, and script serialization. To keep this PR reviewable, I recommend limiting it to Hive inspector conversion plus UDF/UDAF/UDTF support, and moving script TRANSFORM support, especially nested no-SerDe output, to a follow-up. Under that split, the three script-specific findings move with the follow-up; this PR would still need the Hive map-key validation and incompatible-runtime-inspector tests. None of the current findings is blocking. Five locally owned resolved threads also have confirmation replies ready.
Findings
6 total: 0 P0, 0 P1, 6 P2, 0 P3.
Non-blocking (P2)
-
Validate normalized Hive map keys —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:879— see inline. -
Validate bounded keys after final no-SerDe conversion —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:291— see inline. -
Exercise incompatible UDAF runtime inspectors —
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFs.scala:649— see inline. -
Exercise incompatible UDF and UDTF runtime inspectors —
sql/hive/src/main/scala/org/apache/spark/sql/hive/hiveUDFEvaluators.scala:183— see inline. -
Cover scalar bounded-string null tokens —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:224— see inline. -
Avoid the full external round trip for nested output —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:291
The new nested path parses directly to CatalystArrayData/MapData/InternalRow, then materializes the complete value as Scala arrays/maps/rows only to convert it straight back. This cost scales with every descendant of every affected output row and amplifies CPU and heap pressure for large nested results. Please apply the bounded-string checks directly to the parsed Catalyst representation, rebuilding maps through validated construction where keys change.See Shared repair plan 1 in the review body.
Shared repair plans
Shared repair plan 1
Covered findings:
- Validate bounded keys after final no-SerDe conversion —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:291 - Avoid the full external round trip for nested output —
sql/core/src/main/scala/org/apache/spark/sql/execution/BaseScriptTransformationExec.scala:291
Recommended change: Replace the Catalyst-to-Scala-to-Catalyst round trip with target-aware recursive conversion over ArrayData, MapData, and InternalRow. Apply CHAR/VARCHAR checks in place and rebuild every map whose keys are converted through a fresh ArrayBasedMapBuilder.
Why this works: The recursive converter preserves parsed Catalyst containers, transforms only descendants that require bounded-string semantics, and inserts converted keys through the validated builder so nulls and equality collisions use Spark's established map-key policy.
Scope: Refactor the no-SerDe complex bounded-string converter and extend its focused script-output coverage for large nested values and bounded map keys.
Compatibility: Valid no-SerDe complex TRANSFORM output, JSON non-string key restoration, CHAR padding, VARCHAR trailing-space handling, and legacy mode behavior remain unchanged.
Risks: Moving bounded checks into the malformed-field wrapper could swallow EXCEED_LIMIT_LENGTH. Rebuilding only outer maps could leave collisions in maps nested beneath arrays, structs, keys, or values. A new recursive converter could accidentally change nullability or ordering for unaffected descendants.
Constraints: CHAR/VARCHAR overflow must retain EXCEED_LIMIT_LENGTH. Malformed JSON and failed non-string key casts must retain the established null-on-error behavior. Map duplicate handling must honor MAP_KEY_DEDUP_POLICY and reject null keys. Legacy and non-CHAR/VARCHAR complex output behavior must remain unchanged.
Success: Complex no-SerDe CHAR/VARCHAR output remains in Catalyst representation throughout bounded conversion. Distinct input keys that normalize to one CHAR/VARCHAR key cannot escape as duplicate-key MapData. Nested arrays, structs, map keys, and map values preserve padding, trimming, null, overflow, and malformed-field semantics.
Re-review status
Prior AI findings: 5 addressed, 0 still present; additional unresolved findings in this review: 6.
New attribution: 0 newly introduced, 6 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
… follow-up PR Script TRANSFORM support (BaseScriptTransformationExec, HiveScriptTransformationExec, and their tests) is moved to a separate JIRA subtask as suggested in review. This PR now covers only Hive inspector conversion and UDF/UDAF/UDTF support.
…est runtime-inspector guards
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 5 prior findings addressed or moved to the Script TRANSFORM follow-up, 2 remaining; 2 findings in this pass (1 newly introduced, 1 late catch).
The new UDF, UDTF, and UDAF mismatch fixtures address the runtime-inspector guard gaps. The three Script TRANSFORM findings moved with that feature.
Remaining prior findings:
- The bounded Hive map-key conversion now uses a validated builder, but the requested Java and writable inspector collision coverage is still missing.
- The PR body says the focused run contains 19 tests (10/6/3); the advertised command currently passes 23 (10/8/5).
The two new findings are inline.
Verification: the focused SPARK-59277 command passed 23/23 tests; git diff --check passed; added-comment and @link/@see scans found no other substantive issues.
… unwrapper, add map-key collision test
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 3 findings addressed, 1 remaining, 0 new.
The struct field-name compatibility check, orphaned typed field unwrapper removal, and corrected 23-test PR description are resolved in cec2eff61be.
One prior coverage finding remains: HiveInspectorSuite.scala:512 wraps the two candidate keys before invoking the unwrapper. That conversion creates equal HiveChar keys and inserts them into a Java HashMap; Hive CHAR equality ignores trailing spaces, so one entry is already gone before unwrapperFor or its ArrayBasedMapBuilder runs. The raw javaMap declared immediately above is unused, and the writable-inspector path is still uncovered. Please pass raw Java and writable maps directly to their inspectors and assert both EXCEPTION and LAST_WIN, so removing the production builder makes the tests fail.
Verification: the focused SPARK-59277 suites passed 23/23; Hive Scalastyle and git diff --check passed; added-comment and @link/@see scans found no other substantive issues.
…rs and both dedup policies
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 0 fully addressed, 1 remaining (partially addressed), 0 new.
The Java-map regression now reaches ArrayBasedMapBuilder and covers both EXCEPTION and LAST_WIN. The prior coverage finding remains partially open: the test still does not exercise the separate writable-string/Text path (HiveInspectors.scala:658), and its LAST_WIN assertion does not verify that v2 is the winning value. Please run the policy matrix for both javaStringObjectInspector/String and writableStringObjectInspector/Text, and assert the final value under LAST_WIN.
Verification: the focused SPARK-59277 suites passed 23/23; Hive Scalastyle and git diff --check passed; added-comment and @link/@see scans found no other substantive issues.
… in map-key collision test
srielau
left a comment
There was a problem hiding this comment.
Re-review status: 1 prior finding addressed, 0 remaining, 0 new.
Commit bf11663218a now runs the map-key collision policy matrix for both javaStringObjectInspector/String and writableStringObjectInspector/Text, and verifies the winning value under LAST_WIN. No other issues found.
Verification: focused SPARK-59277 suites passed 23/23; main and test Hive Scalastyle and git diff --check passed; added-comment and @link/@see scans found no substantive issues. GitHub Build is still running.
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The six findings from the previous review are resolved in the pinned source: bounded Hive map keys now use validated construction, incompatible UDF/UDTF/UDAF inspectors have call-site coverage, and Script TRANSFORM has moved to its follow-up with the orphan in-place overload removed.
This round found one late-caught, non-blocking correctness issue in a shared helper. Hive ORC uses the DataType-aware unwrapper to preserve nested nanosecond timestamps, so the new unconditional CHAR/VARCHAR branches can also force padding on an adjacent bounded string when read-side padding is explicitly disabled. The Hive-function changes otherwise remain consistent with their captured analysis-time contracts and legacy fallback.
Findings
1 total: 0 P0, 0 P1, 1 P2, 0 P3.
Non-blocking (P2)
- Preserve Hive ORC read-side CHAR padding policy —
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveInspectors.scala:872— see inline.
Re-review status
Prior AI findings: 6 addressed, 0 still present; additional unresolved findings in this review: 1.
New attribution: 0 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
| null | ||
| } | ||
| } | ||
| case (_, c: CharType) => |
There was a problem hiding this comment.
Non-blocking (P2): This typed unwrapper is also used by Hive ORC whenever a complex field contains a nanosecond timestamp. With preserve-only types and spark.sql.readSideCharPadding=false, recursion now reaches this unconditional CHAR branch and pads an adjacent nested value anyway; the same CHAR remains raw when the unrelated nanos sibling is absent. Please separate the ORC nanos-only conversion policy from the strict Hive-function CHAR/VARCHAR policy, propagate that choice through nested containers, and cover this disabled-padding ORC interaction.
Recommended change: Separate nanos-only recursive conversion from Hive-function bounded-string enforcement, make the selected policy propagate through array/map/struct recursion, route the Hive ORC nanos path through the nanos-only policy, and add caller-level coverage for preserve-only read padding disabled alongside nested nanos timestamps.
Why this works: Introduce an explicit internal conversion policy or distinct recursive entry point in HiveInspectors. Hive function evaluators select the policy that applies CHAR/VARCHAR read checks and validated bounded map-key conversion. Hive ORC selects the policy that preserves nanos timestamps but delegates CHAR/VARCHAR leaves to raw untyped extraction; ApplyCharTypePadding remains the authority that conditionally pads table reads. Ensure every recursive container call carries the selected policy rather than falling back to the all-checks default.
Scope: Disentangle Hive function result validation from Hive ORC nanos decoding without changing either feature's supported type surface.
Compatibility: Legacy disabled-mode Hive function results remain unbounded StringType; enabled Hive function boundaries remain checked first-class CHAR/VARCHAR; Script TRANSFORM remains excluded.
Risks: A policy flag that is not propagated into nested arrays, maps, and structs would leave the regression in deeper shapes. Disabling bounded checks for Hive function callers would lose required padding, length enforcement, and normalized map-key validation. Changing the nanos timestamp arms could reintroduce microsecond truncation in Hive ORC.
Constraints: Hive UDF/UDAF/UDTF outputs analyzed as CHAR/VARCHAR must retain their declared kind, length, padding, default collation, and overflow behavior. Hive ORC must preserve TimestampNTZNanosType and TimestampLTZNanosType precision recursively. Table-read padding must continue to follow readSideCharPadding, with standard semantics overriding a disabled setting through ApplyCharTypePadding.
Success: In preserve-only mode with readSideCharPadding=false, a Hive ORC CHAR/VARCHAR value remains raw whether or not an adjacent nested field uses a nanos timestamp. When read-side padding is enabled or standard semantics requires it, table reads still expose the expected CHAR padding through the established logical padding rule. Hive UDF/UDAF/UDTF typed conversion continues to enforce bounded-string padding, overflow, runtime-inspector compatibility, and duplicate normalized map keys. Hive ORC recursively round-trips both nanos timestamp kinds without precision loss.
What changes were proposed in this pull request?
Support first-class CHAR/VARCHAR across Hive extension boundaries when first-class semantics are enabled:
Hive object inspectors cannot carry Spark collation metadata. Collated inputs are accepted, while Hive function results retain the CHAR/VARCHAR length with the default collation.
Script TRANSFORM CHAR/VARCHAR support (with and without SerDe) has been moved to a follow-up subtask: SPARK-59683 / #58936.
JIRA: https://issues.apache.org/jira/browse/SPARK-59277
Why are the changes needed?
With
spark.sql.charVarchar.standardSemantics.enabled=true, Catalyst keeps CHAR/VARCHAR as first-class types. Hive extension paths still handled them as unsupported types or downgraded Hive CHAR/VARCHAR results to STRING. This prevented Hive UDF/UDAF/UDTF functions from operating consistently on scalar, collated, and nested CHAR/VARCHAR values.Does this PR introduce any user-facing change?
Yes. When first-class CHAR/VARCHAR semantics are enabled, Hive UDF/UDAF/UDTF functions now accept and return CHAR/VARCHAR values while preserving declared lengths. Legacy flag-off behavior remains unchanged.
How was this patch tested?
Added focused coverage for:
Ran:
These suites currently contain 23 uniquely named SPARK-59277 tests (HiveInspectorSuite 10, HiveUDFSuite 8, HiveUDAFSuite 5). Scala style checks passed.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Auto