cloud-fan commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4130213665
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ApproxTopKExpressions.scala:
##########
@@ -127,7 +130,9 @@ case class ApproxTopKEstimate(state: Expression, k:
Expression)
override protected def withNewChildrenInternal(newState: Expression, newK:
Expression)
: Expression = copy(state = newState, k = newK)
- override def nullable: Boolean = false
+ // The sketch state is an ordinary nullable input column:
`approx_top_k_estimate(NULL, k)`
+ // returns NULL rather than failing, so the result is nullable whenever the
state is.
+ override def nullable: Boolean = state.nullable
Review Comment:
**Nit (P3):** This is now a two-branch nullability contract, but the added
test asserts only the nullable branch. An implementation that always returns
`true` would still pass, so please also assert that an estimate over the common
non-nullable `approx_top_k_accumulate` state remains non-nullable.
**Verification:**
- **Regression:** Assert both true and false outcomes of the
result-nullability contract using nullable and non-nullable sketch-state
expressions.
- **Compatibility:** Confirm the ordinary aggregate-produced sketch path
retains non-nullable result metadata.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ApproxTopKExpressions.scala:
##########
@@ -109,6 +109,9 @@ case class ApproxTopKEstimate(state: Expression, k:
Expression)
ApproxTopK.checkExpressionNotNull(k, "k")
// eval
val stateEval = left.eval(input)
+ if (stateEval == null) {
Review Comment:
**Non-blocking (P2):** This return runs before `right.eval(input)` and
`ApproxTopK.checkK`, so `approx_top_k_estimate(NULL, 0)` now silently returns
NULL instead of raising `APPROX_TOP_K_NON_POSITIVE_ARG`. The positive-k
contract should still be enforced when the sketch is NULL.
**Verification:**
- **Regression:** Exercise foldable and row-evaluated NULL sketches with a
non-positive k and assert the structured positive-k error.
- **Behavior:** Exercise a NULL sketch with a valid k and a valid sketch
with representative parameters to confirm NULL propagation and normal
estimation remain intact.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala:
##########
@@ -905,6 +905,10 @@ case class ApproxTopKCombine(
*/
override def update(buffer: CombineInternal[Any], input: InternalRow):
CombineInternal[Any] = {
val inputState = state.eval(input).asInstanceOf[InternalRow]
+ if (inputState == null) {
+ // A NULL sketch contributes nothing, like NULL inputs to any other
aggregate.
Review Comment:
**Nit (P3):** This comparison is too broad: aggregates such as `First` can
retain NULL inputs. Please describe only this combine operation's intended
behavior, for example that a NULL sketch contributes nothing.
**Verification:**
- **Inspection:** Confirm the changed text describes only this combine
operation and makes no universal claim contradicted by aggregates that retain
NULLs.
--
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]
