[SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs #41203

RyanBerti · 2023-05-17T16:31:53Z

What changes were proposed in this pull request?

This PR fixes a null pointer thrown when non-foldable values are provided to the lgConfigK or allowDifferentLgConfigK arguments of the hll_sketch_agg, hll_union_agg, or hll_union functions.

Why are the changes needed?

We should be handling this error gracefully, rather than throwing a null pointer exception.

Does this PR introduce any user-facing change?

This PR introduces two new error messages, replacing what would be null pointer exceptions.

How was this patch tested?

Three new negative test cases have been added to the 'SPARK-16484: hll_*_agg + hll_union negative tests' test of the DataframeAggregateSuite.

RyanBerti · 2023-05-17T16:35:19Z

@bersprockets here are the changes to handle non-foldable input args, based on our conversation in #40615. cc @dtenedor @mkaravel

dtenedor · 2023-05-17T16:48:59Z

@RyanBerti thanks for the update!

dtenedor

LGTM except one suggestion to dedup two similar code blocks.

dtenedor · 2023-05-17T16:50:06Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/datasketchesAggregates.scala

@@ -265,6 +288,26 @@ case class HllUnionAgg(
  override protected def withNewChildrenInternal(newLeft: Expression, newRight: Expression):
  HllUnionAgg = copy(left = newLeft, right = newRight)

+  // Overrides for ExpectsInputTypes
+
+  override def checkInputDataTypes(): TypeCheckResult = {


this looks mostly duplicated with L104-120 above. Can we dedup into one place and just call the helper twice instead?

I think it's possible, but might be more valuable to create a new trait that extends ExpectsInputTypes that by default allows all of the types defined by inputTypes to be non-foldable, but provides a mechanism to override each type and define whether its foldable/non-foldable. Feels like this could be utilized by other functions too - thoughts?

Could we define e.g.

private def checkInputDataTypesForHllFunction( defaultCheck: TypeCheckResult, right: Expression): TypeCheckResult = { if (defaultCheck.isFailure) { defaultCheck } else if (!right.foldable) { DataTypeMismatch( errorSubClass = "NON_FOLDABLE_INPUT", messageParameters = Map( "inputName" -> "lgConfigK", "inputType" -> toSQLType(right.dataType), "inputExpr" -> toSQLExpr(right) ) ) } else { TypeCheckSuccess } }

dtenedor · 2023-05-17T17:41:55Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/datasketchesAggregates.scala

@@ -265,6 +288,26 @@ case class HllUnionAgg(
  override protected def withNewChildrenInternal(newLeft: Expression, newRight: Expression):
  HllUnionAgg = copy(left = newLeft, right = newRight)

+  // Overrides for ExpectsInputTypes
+
+  override def checkInputDataTypes(): TypeCheckResult = {


Could we define e.g.

private def checkInputDataTypesForHllFunction( defaultCheck: TypeCheckResult, right: Expression): TypeCheckResult = { if (defaultCheck.isFailure) { defaultCheck } else if (!right.foldable) { DataTypeMismatch( errorSubClass = "NON_FOLDABLE_INPUT", messageParameters = Map( "inputName" -> "lgConfigK", "inputType" -> toSQLType(right.dataType), "inputExpr" -> toSQLExpr(right) ) ) } else { TypeCheckSuccess } }

…nctions

RyanBerti · 2023-05-17T19:42:00Z

@dtenedor I just pushed a commit that tries to generalize the foldable check, as I'm seeing duplicate code in the datasketches functions as well as others (see ApproxCountDistinctForIntervals, ApproximatePercentile, CountMinSketchAgg, etc). I'm open to modifying the trait as needed, or reverting the commit and implementing your suggestion.

dtenedor · 2023-05-17T19:53:27Z

The new trait looks good. In the future we can think about reusing it.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExpectsInputTypes.scala

…essions/ExpectsInputTypes.scala Co-authored-by: Gera Shegalov <gshegalov@nvidia.com>

gerashegalov

LGTM

RyanBerti · 2023-05-23T18:00:17Z

@HyukjinKwon could you help us merge this one?

MaxGekk · 2023-05-25T19:49:02Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

+      )
+      checkAnswer(res, Nil)
+    }
+    assert(error8.toString contains "UNEXPECTED_INPUT_FOLDABLE_VALUE")


Could you use checkError for checking the error class and other fields, please.

I'm seeing various other negative test cases in this suite that use assert, and no examples of checkError. Is there a good example of checkError I could reference?

The HiveUDAF suite uses it pretty extensively.

MaxGekk · 2023-05-25T19:49:20Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

+      )
+      checkAnswer(res, Nil)
+    }
+    assert(error9.toString contains "UNEXPECTED_INPUT_FOLDABLE_VALUE")


ditto: Use checkError

MaxGekk · 2023-05-25T19:49:58Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

+      )
+      checkAnswer(res, Nil)
+    }
+    assert(error10.toString contains "UNEXPECTED_INPUT_FOLDABLE_VALUE")


Please, use checkError

MaxGekk · 2023-05-25T20:01:09Z

core/src/main/resources/error/error-classes.json

@@ -471,6 +471,11 @@
          "class <className> not found."
        ]
      },
+      "UNEXPECTED_INPUT_FOLDABLE_VALUE" : {
+        "message" : [
+          "Parameter <paramIndex> requires a foldable value of <requiredFoldable>, however <inputSql> has a foldable value of <inputFoldable>."


From user's point of view, the message is not clear. What's the foldable value of true?

Can't you just re-use the existing error class: NON_FOLDABLE_INPUT?

Hi @MaxGekk - I introduced the new error message as the text for NON_FOLDABLE_INPUT only makes sense when we expect the input to be foldable, but it is not. I tried to mimic the ExpectsInputTypes.checkInputTypes implementation in ExpectsInputTypesAndFoldable, which checks the foldable attribute of the expression against a user-provided expected value; this allows the trait to check if the foldable value is true OR false, and thus I needed an error message that was more general than NON_FOLDABLE_INPUT. I wonder if we'll ever want to make sure that an input expression isn't foldable (IE for this function arg, you should only ever specify a column)? If not, then maybe I can update the ExpectsInputTypesAndFoldable to only check for cases where we expect an expression to be foldable, and then re-use the NON_FOLDABLE_INPUT error message.

when we expect the input to be foldable, but it is not

I wonder when we need to raise an error when the expected input is any expr (means either foldable or non-foldable ), but it is foldable actually. What's the case?

I'm not sure if there's a clear use case where we'd want to check if the input is not foldable, but this implementation + the new error message does provide that level of flexibility.

Yeah I think non-foldable might be enough, I can't think of a case where we would not want to allow a foldable input. This could also simplify the code a little bit in the checker since Seq[Option[Bool]] could become Seq[Bool]

MaxGekk · 2023-05-27T09:50:16Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/datasketchesAggregates.scala

+
+  override def inputTypes: Seq[AbstractDataType] = Seq(BinaryType, BooleanType)
+
+  override def inputIsFoldable: Seq[Option[Boolean]] = Seq(None, Some(true))


None means that you don't care of it is foldable or not, correct? You just require to be foldable for the second parameter.

Yes, that's correct.

RyanBerti · 2023-06-06T00:04:53Z

@MaxGekk do you still think this PR needs changes, based on our discussion?

holdenk

Overall LGTM provided the existing points raised but others are addressed.

holdenk · 2023-06-06T23:33:34Z

core/src/main/resources/error/error-classes.json

@@ -471,6 +471,11 @@
          "class <className> not found."
        ]
      },
+      "UNEXPECTED_INPUT_FOLDABLE_VALUE" : {
+        "message" : [
+          "Parameter <paramIndex> requires a foldable value of <requiredFoldable>, however <inputSql> has a foldable value of <inputFoldable>."


Yeah I think non-foldable might be enough, I can't think of a case where we would not want to allow a foldable input. This could also simplify the code a little bit in the checker since Seq[Option[Bool]] could become Seq[Bool]

holdenk · 2023-06-06T23:35:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExpectsInputTypes.scala

+ * a value of None bypasses the check for the given column
+ */
+trait ExpectsInputTypesAndFoldable extends ExpectsInputTypes {
+  def inputIsFoldable: Seq[Option[Boolean]]


I know it's a little bike-sheddy so feel free to disregard but I'd have a small preference towards "requireFoldableInputs" or something since as it stands it sounds like this function is returning if the inputs are foldable rather than the required state.

holdenk · 2023-06-06T23:36:22Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

+      )
+      checkAnswer(res, Nil)
+    }
+    assert(error8.toString contains "UNEXPECTED_INPUT_FOLDABLE_VALUE")


The HiveUDAF suite uses it pretty extensively.

github-actions · 2023-09-15T00:17:25Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

Update hll function type checks to also check for non-foldable inputs

20d414a

github-actions bot added the SQL label May 17, 2023

RyanBerti changed the title ~~Update hll function type checks to also check for non-foldable inputs~~ [SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs May 17, 2023

dtenedor approved these changes May 17, 2023

View reviewed changes

Introduce ExpectsInputTypesAndFoldable, utilize it in datasketches fu…

4d6dd31

…nctions

github-actions bot added the CORE label May 17, 2023

gerashegalov reviewed May 18, 2023

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExpectsInputTypes.scala Outdated Show resolved Hide resolved

RyanBerti and others added 2 commits May 18, 2023 10:13

Fix error message formatting

87be383

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expr…

a169cec

…essions/ExpectsInputTypes.scala Co-authored-by: Gera Shegalov <gshegalov@nvidia.com>

gerashegalov approved these changes May 18, 2023

View reviewed changes

MaxGekk requested changes May 25, 2023

View reviewed changes

MaxGekk reviewed May 27, 2023

View reviewed changes

holdenk reviewed Jun 6, 2023

View reviewed changes

github-actions bot added the Stale label Sep 15, 2023

github-actions bot closed this Sep 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs #41203

[SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs #41203

RyanBerti commented May 17, 2023

RyanBerti commented May 17, 2023

dtenedor commented May 17, 2023

dtenedor left a comment

dtenedor May 17, 2023

RyanBerti May 17, 2023

dtenedor May 17, 2023 •

edited

dtenedor May 17, 2023 •

edited

RyanBerti commented May 17, 2023 •

edited

dtenedor commented May 17, 2023

gerashegalov left a comment

RyanBerti commented May 23, 2023

MaxGekk May 25, 2023

RyanBerti Jun 6, 2023

holdenk Jun 6, 2023

MaxGekk May 25, 2023

MaxGekk May 25, 2023

MaxGekk May 25, 2023

RyanBerti May 25, 2023 •

edited

MaxGekk May 27, 2023 •

edited

RyanBerti May 30, 2023

holdenk Jun 6, 2023

MaxGekk May 27, 2023

RyanBerti May 30, 2023

RyanBerti commented Jun 6, 2023

holdenk left a comment

holdenk Jun 6, 2023

holdenk Jun 6, 2023

holdenk Jun 6, 2023

github-actions bot commented Sep 15, 2023


		override def inputTypes: Seq[AbstractDataType] = Seq(BinaryType, BooleanType)

		override def inputIsFoldable: Seq[Option[Boolean]] = Seq(None, Some(true))

[SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs #41203

[SPARK-16484][SQL] Update hll function type checks to also check for non-foldable inputs #41203

Conversation

RyanBerti commented May 17, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

RyanBerti commented May 17, 2023

dtenedor commented May 17, 2023

dtenedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtenedor May 17, 2023 • edited

Choose a reason for hiding this comment

dtenedor May 17, 2023 • edited

Choose a reason for hiding this comment

RyanBerti commented May 17, 2023 • edited

dtenedor commented May 17, 2023

gerashegalov left a comment

Choose a reason for hiding this comment

RyanBerti commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanBerti May 25, 2023 • edited

Choose a reason for hiding this comment

MaxGekk May 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanBerti commented Jun 6, 2023

holdenk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 15, 2023

dtenedor May 17, 2023 •

edited

dtenedor May 17, 2023 •

edited

RyanBerti commented May 17, 2023 •

edited

RyanBerti May 25, 2023 •

edited

MaxGekk May 27, 2023 •

edited