[SPARK-57848][SQL] Support the TIME data type in approx_top_k#57013
[SPARK-57848][SQL] Support the TIME data type in approx_top_k#57013iRakson wants to merge 1 commit into
Conversation
shrirangmhalgi
left a comment
shrirangmhalgi
left a comment
There was a problem hiding this comment.
The implementation is correct - all 5 dispatch sites (isValidItemType, newItemsSketch, newArrayOfItemsSerDe, update, getItem) consistently use Long + ArrayOfLongsSerDe, matching TimeType's nanosecond internal representation. The positive combine path is covered via the existing gridTest infrastructure.
nit (non blocking): The positive test uses whole-second literals (TIME'06:00:00'). Consider adding one sub-second value (e.g. TIME'08:30:00.123456') to verify the sketch discriminates at fractional-second granularity.
|
@MaxGekk Please review. |
|
@cloud-fan Please review |
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
Clean, correct extension of the Long-backed type-dispatch pattern; the one non-blocking item is a test-coverage gap.
Suggestions (1)
- ApproxTopKSuite.scala:524:
mixedTimeTypeshas a single TIME(6) entry, so the cross-precision combine path is untested — see inline
Verification
Confirmed all five dispatch sites (isDataTypeSupported, createItemsSketch, genSketchSerDe, buffer update, buffer eval) route TimeType to Long + ArrayOfLongsSerDe, matching its nanosecond internal representation, and that getResultDataType + the DDL state round-trip preserve time(p) precision. The three no-catch-all match blocks each received the new type, so no MatchError is reachable. The new comments' claim — TIME is incomparable with DATE/TIMESTAMP/TIMESTAMP_NTZ (fails at UNION) while those three widen (fail at combine) — checks out against TypeCoercionHelper.findWiderDateTimeType (TIME-vs-non-TIME returns None; two TIMEs widen to the max precision).
| // mutually widen and so only fail at approx_top_k_combine runtime, not at UNION analysis), so | ||
| // it cannot join mixedDateTimeTypes: "among different ... types - fail at combine" below | ||
| // asserts every pair in that Seq unions successfully and only fails at the runtime combine. | ||
| val mixedTimeTypes: Seq[(DataType, String, Seq[String])] = Seq( |
There was a problem hiding this comment.
mixedTimeTypes has only a single TIME(6) entry, so it can't exercise the TIME analogue of the "among different ... types - fail at combine" test. Two different-precision TIMEs (e.g. TIME(3) vs TIME(6)) widen at UNION via findWiderDateTimeType (TimeType(max(p1,p2))), so the UNION succeeds — but the two sketch states carry time(3) vs time(6), which updateItemDataType rejects with != at combine runtime (APPROX_TOP_K_SKETCH_TYPE_NOT_MATCH). That "widens at UNION, fails at combine on precision" path is the exact TIME parallel of the tested datetime behavior and is currently uncovered. Adding a second, different-precision TIME entry here would close it. Non-blocking — the behavior is correct; this is coverage only.
3 participants
What changes were proposed in this pull request?
Add support for TimeType to approx_top_k. After this change approx_top_k accepts TimeType and returns correct top-k time values and columns.
Why are the changes needed?
The TIME data type was added by SPIP SPARK-51162 and shipped in Spark 4.1.0, but the approx_top_k aggregate functions did not accept it. Calling approx_top_k on a TIME column failed with a type-check error, even though the operation is well-defined. This PR closes that gap.
Does this PR introduce any user-facing change?
Yes, Now users can use approx_top_k over TimeType columns as well.
How was this patch tested?
UTs added.
Was this patch authored or co-authored using generative AI tooling?
Used claude code for UTs.