holdenk commented on code in PR #41203:
URL: https://github.com/apache/spark/pull/41203#discussion_r1220539520
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExpectsInputTypes.scala:
##########
@@ -74,3 +74,41 @@ object ExpectsInputTypes extends QueryErrorsBase {
trait ImplicitCastInputTypes extends ExpectsInputTypes {
// No other methods
}
+
+/**
+ * An extension of the ExpectsInputTypes trait that also checks each
+ * input expression's foldable attribute against inputIsFoldable;
+ * inputInputFoldable is a Seq of type Option[Boolean], specifying
+ * a value of None bypasses the check for the given column
+ */
+trait ExpectsInputTypesAndFoldable extends ExpectsInputTypes {
+ def inputIsFoldable: Seq[Option[Boolean]]
Review Comment:
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.
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala:
##########
@@ -1876,6 +1876,49 @@ class DataFrameAggregateSuite extends QueryTest
checkAnswer(res, Nil)
}
assert(error7.toString contains "UnsupportedOperationException")
+
+ // validate specific args require foldable = true
+ val error8 = intercept[AnalysisException] {
+ val res = sql(
+ """with cte as (
+ |select * from values
+ | (1, 12)
+ | as tab(col, logk)
+ |)
+ |select hll_sketch_agg(col, logk) from cte
+ |""".stripMargin
+ )
+ checkAnswer(res, Nil)
+ }
+ assert(error8.toString contains "UNEXPECTED_INPUT_FOLDABLE_VALUE")
Review Comment:
The HiveUDAF suite uses it pretty extensively.
##########
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>."
Review Comment:
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]
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
