You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jubins (via GitHub)" <gi...@apache.org> on 2026/09/28 06:35:00 UTC

[PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

jubins opened a new pull request, #59094:
URL: https://github.com/apache/spark/pull/59094

   ### What changes were proposed in this pull request?
   
   Fixes [SPARK-59818](https://issues.apache.org/jira/browse/SPARK-59818) — makes `approx_top_k_estimate` and `approx_top_k_combine` handle a NULL sketch state instead of throwing `NullPointerException`. Two related gaps are addressed:
   
   **`approx_top_k_estimate` dereferenced the state unconditionally.** `ApproxTopKEstimate` overrides `BinaryExpression.eval` directly rather than implementing `nullSafeEval`, so the inherited null handling is bypassed, and `eval` calls `stateEval.asInstanceOf[InternalRow].getBinary(0)` with no null check. It also declared `override def nullable: Boolean = false` even though the result is NULL whenever the state is, so the reported output schema was wrong and the optimizer reasoned incorrectly about the expression.
   
   **`approx_top_k_combine` had the same unguarded dereference in `update`.** `ApproxTopKCombine.update` calls `inputState.getBinary(0)` directly, so a NULL sketch fails on the executors and aborts the running job, rather than being ignored the way aggregates normally ignore NULL inputs.
   
   Analysis does not prevent either case: `checkStateFieldAndType` validates only the *schema* of the state struct, not its nullness, so a correctly-typed nullable struct column passes the analyzer and then fails at optimization or execution time. A NULL sketch arises naturally from an outer join, a `UNION` with a missing side, or a partially-populated column.
   
   Both are surfaced and locked down by new tests.
   
   **Brief change log**
   
   - `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ApproxTopKExpressions.scala`: `ApproxTopKEstimate.eval` returns `null` when the state evaluates to `null`, and `nullable` is now `state.nullable` instead of a hardcoded `false`, so the reported schema matches the values actually produced.
   - `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala`: `ApproxTopKCombine.update` returns the buffer untouched for a NULL sketch, so combine ignores NULL inputs like any other aggregate.
   - `sql/core/src/test/scala/org/apache/spark/sql/ApproxTopKSuite.scala`: added four tests covering the foldable NULL state, the non-foldable NULL state, the reported nullability, and combine skipping NULL sketches.
   
   ### Why are the changes needed?
   
   Both paths fail on a valid query.
   
   `approx_top_k_estimate` raises the NPE inside `ConstantFolding`, which reports it to the user as an internal error:
   
   ```sql
   SELECT approx_top_k_estimate(s, 5) FROM (
     SELECT CAST(NULL AS STRUCT<sketch: BINARY, maxItemsTracked: INT,
       itemDataType: INT, itemDataTypeDDL: STRING>) AS s
   );
   ```
   
   ```
   [INTERNAL_ERROR] The Spark SQL phase optimization failed with an internal error.
   You hit a bug in Spark or the Spark plugins you use. SQLSTATE: XX000
   
   Caused by: java.lang.NullPointerException: Cannot invoke
     "org.apache.spark.sql.catalyst.InternalRow.getBinary(int)" because "stateEval" is null
     at ApproxTopKEstimate.eval(ApproxTopKExpressions.scala:113)
     at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$.tryFold(expressions.scala:65)
   ```
   
   `approx_top_k_combine` fails on the executors and aborts the job:
   
   ```sql
   SELECT approx_top_k_estimate(approx_top_k_combine(s), 5) FROM (
     SELECT CAST(NULL AS STRUCT<sketch: BINARY, maxItemsTracked: INT,
       itemDataType: INT, itemDataTypeDDL: STRING>) AS s
   );
   ```
   
   ```
   org.apache.spark.SparkException: Job aborted due to stage failure:
     Task 0 in stage 0.0 failed 1 times ...
   Caused by: java.lang.NullPointerException
     at ApproxTopKCombine.update(ApproxTopKAggregates.scala:908)
     at TypedImperativeAggregate.update(interfaces.scala:583)
   ```
   
   The new behaviour follows ordinary SQL semantics for a nullable input column: `approx_top_k_estimate(NULL, k)` returns NULL, and `approx_top_k_combine` skips NULL sketches.
   
   This is distinct from the existing `APPROX_TOP_K_NULL_ARG` error, which is raised for NULL *foldable configuration* arguments (`k`, `maxItemsTracked`). That check, `ApproxTopK.checkExpressionNotNull`, calls `expr.eval()` with no input row, so it applies only to literals and is not applicable to a per-row data column.
   
   It is also distinct from earlier NULL-related work on these functions. SPARK-53960 added handling for NULLs *within* the accumulated data (counting them as items) and did not touch the NULL-state path or the `nullable` declaration; SPARK-58095 concerns *empty* input (zero rows) in combine's `eval`/`serialize`, which is a different code path and trigger.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes, in three ways, all aligning the functions with ordinary SQL null semantics:
   
   1. `approx_top_k_estimate` on a NULL sketch state returns NULL instead of failing the query with `[INTERNAL_ERROR]`.
   2. `approx_top_k_combine` skips NULL sketches instead of aborting the job with a `NullPointerException`.
   3. `approx_top_k_estimate` now reports itself as nullable when its state argument is nullable. Previously it always reported `nullable = false`, which was incorrect for a nullable state. The reported schema is unchanged for a non-nullable state, which is the common case — the `sql-expression-schema.md` golden file is unaffected, as its example applies the function to an aggregate result.
   
   These are changes compared to released versions: `approx_top_k` is annotated `since = "4.1.0"`.
   
   ### How was this patch tested?
   
   Four new tests in `ApproxTopKSuite`, covering both functions:
   
   - `SPARK-59818: estimate of a foldable NULL state returns NULL`
   - `SPARK-59818: estimate of a non-foldable NULL state returns NULL`
   - `SPARK-59818: estimate is nullable when its state is nullable`
   - `SPARK-59818: combine skips NULL sketches`
   
   The foldable and non-foldable state are covered separately because only the former is reached by `ConstantFolding`; the non-foldable case is built with a `UNION ALL` so the state stays a per-row column.
   
   All four tests were confirmed to fail without the fix. Reverting the two source commits and re-running them produces exactly the reported failures — `Cannot invoke "InternalRow.getBinary(int)" because "stateEval" is null` for the three estimate tests and `... because "inputState" is null`, aborting the stage, for the combine test — and all four pass with the fix applied.
   
   The full `ApproxTopKSuite` passes (200 tests: 196 existing plus the 4 new). `ExpressionInfoSuite`, `NullabilitySuite`, `CanonicalizeSuite`, and `ExpressionsSchemaSuite` also pass, confirming the `nullable` change leaves the expression schema golden file unchanged.
   
   Run:
   
   ```
   build/sbt 'sql/testOnly org.apache.spark.sql.ApproxTopKSuite'
   build/sbt 'catalyst/testOnly *ExpressionInfoSuite *NullabilitySuite *CanonicalizeSuite'
   build/sbt 'sql/testOnly *ExpressionsSchemaSuite'
   ```
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   - [x] Yes - Claude Code was used as a pair-programming assistant. All changes were reviewed and verified by the author. Generated-by: Claude Opus 5


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4148233008


##########
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 carries no items, so it contributes nothing to the combined sketch.
+      return buffer

Review Comment:
   couldn't reproduce this, I tried separate partials via grouping keys, shuffle partitions of 1/2/8/64 with AQE disabled, and 200 NULL rows against one real sketch, all returning identical correct results, which I think is because `uncheckedItemDataType` comes from `state.dataType` and is one fixed type per query, so every partial stamps the same type. added your verification cases as tests in d6d289d.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6017041031

   Thank you for iterating on this! Should be good to go now
   
   However, let's wait for @cloud-fan / @HyukjinKwon to re-review as well


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6299954854

   @cloud-fan @HyukjinKwon is this good to merge?


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6100646849

   @HyukjinKwon @cloud-fan could you please re-review?


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-5899868919

   > ## Review summary
   > The NULL-state handling fixes the reported estimate and combine crashes, but the estimate's early return also bypasses validation of a non-positive `k`, allowing invalid input to succeed silently. The new combine comment overstates aggregate NULL behavior, and the nullability tests do not protect the non-nullable branch of the new metadata contract. These three issues should be addressed before completion.
   > 
   > ## Findings
   > **3 total: 0 P0, 0 P1, 1 P2, 2 P3.**
   > 
   > ### Non-blocking (P2)
   > * **Validate k before returning for a NULL sketch** — `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ApproxTopKExpressions.scala:112` — see inline.
   > 
   > ### Nit (P3)
   > * **Avoid claiming that all aggregates ignore NULLs** — `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala:909` — see inline.
   > * **Cover the non-nullable nullability branch** — `sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ApproxTopKExpressions.scala:135` — see inline.
   
   thanks for your review @cloud-fan, all three comments addressed


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6335133719

   hi @cloud-fan is this good to merge? please let me know if further comments
   
   


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4139328069


##########
sql/core/src/test/scala/org/apache/spark/sql/ApproxTopKSuite.scala:
##########
@@ -549,6 +549,71 @@ class ApproxTopKSuite extends SharedSparkSession {
     )
   }
 
+
+  private val nullSketchState =
+    """CAST(NULL AS STRUCT<sketch: BINARY, maxItemsTracked: INT,
+      |  itemDataType: INT, itemDataTypeDDL: STRING>)""".stripMargin
+
+  test("SPARK-59818: estimate of a foldable NULL state returns NULL") {
+    checkAnswer(sql(s"SELECT approx_top_k_estimate($nullSketchState, 5)"), Row(null))
+  }
+
+  test("SPARK-59818: estimate of a non-foldable NULL state returns NULL") {
+    withTempView("estimate_null_state") {
+      sql(
+        s"""SELECT approx_top_k_accumulate(expr) AS state
+           |FROM VALUES 0, 1, 1 AS tab(expr)
+           |UNION ALL
+           |SELECT $nullSketchState AS state""".stripMargin)
+        .createOrReplaceTempView("estimate_null_state")
+      val res = sql("SELECT approx_top_k_estimate(state, 2) FROM estimate_null_state")
+      checkAnswer(res, Seq(Row(Seq(Row(1, 2), Row(0, 1))), Row(null)))
+    }
+  }
+
+  test("SPARK-59818: estimate nullability follows the nullability of its state") {
+    withTempView("estimate_nullable_state") {
+      sql(s"SELECT $nullSketchState AS state")
+        .createOrReplaceTempView("estimate_nullable_state")
+      val nullableRes =
+        sql("SELECT approx_top_k_estimate(state, 2) FROM estimate_nullable_state")
+      assert(nullableRes.schema.fields.head.nullable)
+    }
+    // The common case: a sketch produced by approx_top_k_accumulate is non-nullable, so the
+    // estimate over it must stay non-nullable.
+    val nonNullableRes = sql(
+      """SELECT approx_top_k_estimate(approx_top_k_accumulate(expr), 2)
+        |FROM VALUES 0, 1, 1 AS tab(expr)""".stripMargin)
+    assert(!nonNullableRes.schema.fields.head.nullable)
+  }
+
+  test("SPARK-59818: estimate rejects a non-positive k even for a NULL state") {
+    Seq(0, -1).foreach { invalidK =>
+      checkError(
+        exception = intercept[SparkRuntimeException] {
+          sql(s"SELECT approx_top_k_estimate($nullSketchState, $invalidK)").collect()
+        },
+        condition = "APPROX_TOP_K_NON_POSITIVE_ARG",
+        parameters = Map("argName" -> "`k`", "argValue" -> invalidK.toString)
+      )
+    }
+    withTempView("estimate_invalid_k") {
+      sql(
+        s"""SELECT approx_top_k_accumulate(expr) AS state

Review Comment:
   **Nit (P3):** This fixture includes a populated sketch that can raise APPROX_TOP_K_NON_POSITIVE_ARG on its own. If k validation moves behind the NULL-state return again, collect() can therefore satisfy this assertion without proving that the row-evaluated NULL state rejects k = 0, so the claimed runtime NULL-path regression coverage can remain green.
   
   **Verification:**
   - **Regression:** Exercise a non-positive k against a row-evaluated, non-foldable NULL sketch state with no alternate row able to throw the expected validation error.



##########
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 carries no items, so it contributes nothing to the combined sketch.
+      return buffer

Review Comment:
   **Non-blocking (P2):** Returning the untouched buffer is not type-neutral once this NULL row is the only input to a partial aggregate: serialization fills the unset itemDataType from the coercion-widened state schema, and final merge can reject it against the real sketch's DDL type. That makes a supported combine over the same NULL and non-NULL states fail with APPROX_TOP_K_SKETCH_TYPE_NOT_MATCH solely because those rows landed in different partial aggregates.
   
   **Verification:**
   - **Regression:** Combine a real sketch with a coercion-widened NULL state isolated in a different partial aggregate and verify the result is independent of partition placement.
   - **Compatibility:** Verify that empty or NULL-only final results retain valid concrete state metadata while incompatible non-NULL sketch types remain rejected.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6125736037

   Thanks for the ping. I have completed the requested re-review and will post the technical feedback separately.
   
   <!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:6100646849","thread_id":"conversation:6100646849","verdict_sha256":"8fd7c5f8f90a68ab25440b76a5bfad9e32460d5c37b3dd38b0dff914c8f94a57"} -->


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4138836830


##########
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:
   updated, the test now asserts both branches, nullable for a nullable state, and non-nullable for the common `approx_top_k_accumulate` produced state



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4148194075


##########
sql/core/src/test/scala/org/apache/spark/sql/ApproxTopKSuite.scala:
##########
@@ -549,6 +549,71 @@ class ApproxTopKSuite extends SharedSparkSession {
     )
   }
 
+
+  private val nullSketchState =
+    """CAST(NULL AS STRUCT<sketch: BINARY, maxItemsTracked: INT,
+      |  itemDataType: INT, itemDataTypeDDL: STRING>)""".stripMargin
+
+  test("SPARK-59818: estimate of a foldable NULL state returns NULL") {
+    checkAnswer(sql(s"SELECT approx_top_k_estimate($nullSketchState, 5)"), Row(null))
+  }
+
+  test("SPARK-59818: estimate of a non-foldable NULL state returns NULL") {
+    withTempView("estimate_null_state") {
+      sql(
+        s"""SELECT approx_top_k_accumulate(expr) AS state
+           |FROM VALUES 0, 1, 1 AS tab(expr)
+           |UNION ALL
+           |SELECT $nullSketchState AS state""".stripMargin)
+        .createOrReplaceTempView("estimate_null_state")
+      val res = sql("SELECT approx_top_k_estimate(state, 2) FROM estimate_null_state")
+      checkAnswer(res, Seq(Row(Seq(Row(1, 2), Row(0, 1))), Row(null)))
+    }
+  }
+
+  test("SPARK-59818: estimate nullability follows the nullability of its state") {
+    withTempView("estimate_nullable_state") {
+      sql(s"SELECT $nullSketchState AS state")
+        .createOrReplaceTempView("estimate_nullable_state")
+      val nullableRes =
+        sql("SELECT approx_top_k_estimate(state, 2) FROM estimate_nullable_state")
+      assert(nullableRes.schema.fields.head.nullable)
+    }
+    // The common case: a sketch produced by approx_top_k_accumulate is non-nullable, so the
+    // estimate over it must stay non-nullable.
+    val nonNullableRes = sql(
+      """SELECT approx_top_k_estimate(approx_top_k_accumulate(expr), 2)
+        |FROM VALUES 0, 1, 1 AS tab(expr)""".stripMargin)
+    assert(!nonNullableRes.schema.fields.head.nullable)
+  }
+
+  test("SPARK-59818: estimate rejects a non-positive k even for a NULL state") {
+    Seq(0, -1).foreach { invalidK =>
+      checkError(
+        exception = intercept[SparkRuntimeException] {
+          sql(s"SELECT approx_top_k_estimate($nullSketchState, $invalidK)").collect()
+        },
+        condition = "APPROX_TOP_K_NON_POSITIVE_ARG",
+        parameters = Map("argName" -> "`k`", "argValue" -> invalidK.toString)
+      )
+    }
+    withTempView("estimate_invalid_k") {
+      sql(
+        s"""SELECT approx_top_k_accumulate(expr) AS state

Review Comment:
   good catch, fixed this in d6d289d. the row-evaluated fixture is now NULL states only, so nothing else can raise the error. confirmed by moving checkK back behind the NULL return, which now fails where the old fixture stayed green.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4138807420


##########
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:
   good catch, fixed NULL-state regression and added tests covering foldable and row-evaluated NULL states.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-5917761843

   > The production fixes look correct. One test gap remains: the row-evaluated invalid-k case mixes a valid sketch with the NULL row, so the valid row can raise the expected error even if NULL rows bypass validation. Please make that assertion depend only on a non-foldable NULL state.
   
   thank you for reviewing @cloud-fan, added tests for both in recent commit


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6011932116

   hi @cloud-fan @HyukjinKwon please let me know if there are further comments, would appreciate if this can be approved/merged. thank you!


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6261414953

   > Thanks for the ping. I have completed the requested re-review and will post the technical feedback separately.
   
   @cloud-fan thanks for the approval, could you please re-review?


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4248827071


##########
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 carries no items, so it contributes nothing to the combined sketch.
+      return buffer

Review Comment:
   The new partition test keeps both the live sketch and NULL state BIGINT, so every stamped partial type agrees. Please use an INT live sketch with a BIGINT typed NULL branch isolated in a partial; existing SPARK-58164 coverage already establishes that a BIGINT-widened state with only an INT real sketch is supported.
   
   <!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4139328077","thread_id":"inline:4139328077","verdict_sha256":"8fd7c5f8f90a68ab25440b76a5bfad9e32460d5c37b3dd38b0dff914c8f94a57"} -->



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "jubins (via GitHub)" <gi...@apache.org>.
jubins commented on code in PR #59094:
URL: https://github.com/apache/spark/pull/59094#discussion_r4138831852


##########
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:
   done, reworded the comment to describe only this operation 



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-5900978189

   The production fixes look correct. One test gap remains: the row-evaluated invalid-k case mixes a valid sketch with the NULL row, so the valid row can raise the expected error even if NULL rows bypass validation. Please make that assertion depend only on a non-foldable NULL state.
   
   <!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:5899868919","thread_id":"conversation:5899868919","verdict_sha256":"6b63c2917281883e3f60bf65eaebe1be2b87a41d6d3ee2fd016a578b153547cc"} -->


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #59094:
URL: https://github.com/apache/spark/pull/59094#issuecomment-6339877113

   **Merge Summary:**
   - merged into master https://github.com/apache/spark/commit/329c057478b5c30fcdee14e1ee3524bffc0d6436
   - merged into branch-4.x https://github.com/apache/spark/commit/f6b98369cdca394ba5d0ffbc5e93495a2bac3364
   
   *Posted by `merge_spark_pr.py`*


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #59094: [SPARK-59818][SQL] Handle a NULL sketch state in approx_top_k_estimate and approx_top_k_combine
URL: https://github.com/apache/spark/pull/59094


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org