uros-b commented on code in PR #57375: URL: https://github.com/apache/spark/pull/57375#discussion_r3621607120
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala: ########## Review Comment: Please investigate a bit more and coordinate with https://github.com/apache/spark/pull/57289 accordingly: The fix closes the intermediate serialize/deserialize crash and correctly handles the mixed case (some empty some non-empty partitions resolve the null type during merge/update, so eval sees a concrete type). But a combine over an entirely empty input leaves the final buffer with itemDataType == null, and eval then calls ApproxTopK.genSketchSerDe(null) (line 900) and dataTypeToDDL(null) (line 902); neither has a null/wildcard case (verified against genSketchSerDe, lines 273-291), so the query still fails with a scala.MatchError. This is pre-existing (the query failed earlier, at serialize, before this PR) and arguably outside the declared "between stages" scope, and is separately handled by draft #57289. The author should decide whether to also cover it here (a fallback to uncheckedItemDataType in eval would close it) or explicitly document the remaining gap and defer to #5728 9; not leave a still-crashing realistic path silently behind a narrower fix. -- 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]
