You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/15 03:52:38 UTC
[GitHub] [spark] beliefer opened a new pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
beliefer opened a new pull request #35213:
URL: https://github.com/apache/spark/pull/35213
### What changes were proposed in this pull request?
Currently, Spark provides `RuntimeReplaceable` to replace function with another function. The last function must be exists in build-in functions, so `RuntimeReplaceable` make Spark could reuse the implement of build-in function.
But `RuntimeReplaceable` not works for aggregate function.
### Why are the changes needed?
Make `RuntimeReplaceable` works for `AggregateFunction`
### Does this PR introduce _any_ user-facing change?
'Yes'.
This PR makes `RuntimeReplaceable` work for `AggregateFunction`.
### How was this patch tested?
Exists tests.
--
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
[GitHub] [spark] cloud-fan commented on a change in pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35213:
URL: https://github.com/apache/spark/pull/35213#discussion_r785718580
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -46,12 +46,11 @@ import org.apache.spark.util.Utils
*/
object ReplaceExpressions extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
- _.containsAnyPattern(RUNTIME_REPLACEABLE, COUNT_IF, BOOL_AGG, REGR_COUNT)) {
+ _.containsAnyPattern(RUNTIME_REPLACEABLE, COUNT_IF, BOOL_AGG)) {
case e: RuntimeReplaceable => e.child
case CountIf(predicate) => Count(new NullIf(predicate, Literal.FalseLiteral))
case BoolOr(arg) => Max(arg)
case BoolAnd(arg) => Min(arg)
Review comment:
Can we remove the above 3 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
[GitHub] [spark] beliefer commented on pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35213:
URL: https://github.com/apache/spark/pull/35213#issuecomment-1013995690
ping @cloud-fan
--
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
[GitHub] [spark] beliefer commented on a change in pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35213:
URL: https://github.com/apache/spark/pull/35213#discussion_r785817488
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -46,12 +46,11 @@ import org.apache.spark.util.Utils
*/
object ReplaceExpressions extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan = plan.transformAllExpressionsWithPruning(
- _.containsAnyPattern(RUNTIME_REPLACEABLE, COUNT_IF, BOOL_AGG, REGR_COUNT)) {
+ _.containsAnyPattern(RUNTIME_REPLACEABLE, COUNT_IF, BOOL_AGG)) {
case e: RuntimeReplaceable => e.child
case CountIf(predicate) => Count(new NullIf(predicate, Literal.FalseLiteral))
case BoolOr(arg) => Max(arg)
case BoolAnd(arg) => Min(arg)
Review comment:
OK
--
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
[GitHub] [spark] cloud-fan commented on a change in pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35213:
URL: https://github.com/apache/spark/pull/35213#discussion_r786032039
##########
File path: sql/core/src/test/resources/sql-tests/results/udf/udf-group-by.sql.out
##########
@@ -377,43 +377,39 @@ struct<udf(udf(k)):int,every:boolean>
-- !query
SELECT every(udf(1))
-- !query schema
-struct<>
+struct<min(udf(1)):int>
-- !query output
-org.apache.spark.sql.AnalysisException
-cannot resolve 'every(CAST(udf(cast(1 as string)) AS INT))' due to data type mismatch: Input to function 'every' should have been boolean, but it's [int].; line 1 pos 7
+1
-- !query
SELECT some(udf(1S))
-- !query schema
-struct<>
+struct<max(udf(1)):smallint>
-- !query output
-org.apache.spark.sql.AnalysisException
-cannot resolve 'some(CAST(udf(cast(1 as string)) AS SMALLINT))' due to data type mismatch: Input to function 'some' should have been boolean, but it's [smallint].; line 1 pos 7
+1
-- !query
SELECT any(udf(1L))
-- !query schema
-struct<>
+struct<max(udf(1)):bigint>
-- !query output
-org.apache.spark.sql.AnalysisException
-cannot resolve 'any(CAST(udf(cast(1 as string)) AS BIGINT))' due to data type mismatch: Input to function 'any' should have been boolean, but it's [bigint].; line 1 pos 7
+1
-- !query
SELECT udf(every("true"))
-- !query schema
-struct<>
+struct<udf(min(true)):string>
-- !query output
-org.apache.spark.sql.AnalysisException
-cannot resolve 'every('true')' due to data type mismatch: Input to function 'every' should have been boolean, but it's [string].; line 1 pos 11
Review comment:
the behavior change is a bit scarying. Can you explain it?
--
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
[GitHub] [spark] beliefer commented on pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35213:
URL: https://github.com/apache/spark/pull/35213#issuecomment-1015256049
It seems the output schema is difficult to be consistent with the previous one.
--
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
[GitHub] [spark] cloud-fan commented on pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35213:
URL: https://github.com/apache/spark/pull/35213#issuecomment-1014234456
> Does this PR introduce any user-facing change?
This is for end-users, not spark developers. It's not a user-facing change because end-users who only use DataFrame/SQL APIs can't notice this change.
--
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
[GitHub] [spark] beliefer edited a comment on pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer edited a comment on pull request #35213:
URL: https://github.com/apache/spark/pull/35213#issuecomment-1015256049
It seems the output schema is difficult to be consistent with the previous one.
https://github.com/apache/spark/pull/35241 is a better way.
--
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
[GitHub] [spark] beliefer commented on pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35213:
URL: https://github.com/apache/spark/pull/35213#issuecomment-1049592190
https://github.com/apache/spark/pull/35534 merged
--
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
[GitHub] [spark] beliefer closed pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
beliefer closed pull request #35213:
URL: https://github.com/apache/spark/pull/35213
--
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
[GitHub] [spark] cloud-fan commented on a change in pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35213:
URL: https://github.com/apache/spark/pull/35213#discussion_r785718131
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##########
@@ -366,6 +366,8 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
// are semantically equal.
override lazy val preCanonicalized: Expression = child.preCanonicalized
+ def isAggregate: Boolean = child.isInstanceOf[AggregateFunction]
Review comment:
More thoughts about it: technically a `RuntimeReplaceable` can combine one or more expressions, even for something like `f(x) = max(x) - min(x)`.
For aggregate functions, it's very hard to reason about `f(distinct x) FILTER WHERE ...` if `f(x)` combines many expressions, so I think it makes sense to only allow a direct mapping here.
We can make this assumption more explicit here
```
def isAggregate: Boolean = {
if (child.isInstanceOf[AggregateFunction]) {
true
} else {
assert(child.find(_.isInstanceOf[AggregateFunction]).isEmpty)
false
}
}
```
--
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
[GitHub] [spark] cloud-fan commented on a change in pull request #35213: [SPARK-37914][SQL] Make `RuntimeReplaceable` works for `AggregateFunction`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35213:
URL: https://github.com/apache/spark/pull/35213#discussion_r786031606
##########
File path: sql/core/src/test/resources/sql-functions/sql-expression-schema.md
##########
@@ -336,17 +336,17 @@
| org.apache.spark.sql.catalyst.expressions.aggregate.BitAndAgg | bit_and | SELECT bit_and(col) FROM VALUES (3), (5) AS tab(col) | struct<bit_and(col):int> |
| org.apache.spark.sql.catalyst.expressions.aggregate.BitOrAgg | bit_or | SELECT bit_or(col) FROM VALUES (3), (5) AS tab(col) | struct<bit_or(col):int> |
| org.apache.spark.sql.catalyst.expressions.aggregate.BitXorAgg | bit_xor | SELECT bit_xor(col) FROM VALUES (3), (5) AS tab(col) | struct<bit_xor(col):int> |
-| org.apache.spark.sql.catalyst.expressions.aggregate.BoolAnd | bool_and | SELECT bool_and(col) FROM VALUES (true), (true), (true) AS tab(col) | struct<bool_and(col):boolean> |
-| org.apache.spark.sql.catalyst.expressions.aggregate.BoolAnd | every | SELECT every(col) FROM VALUES (true), (true), (true) AS tab(col) | struct<every(col):boolean> |
-| org.apache.spark.sql.catalyst.expressions.aggregate.BoolOr | any | SELECT any(col) FROM VALUES (true), (false), (false) AS tab(col) | struct<any(col):boolean> |
-| org.apache.spark.sql.catalyst.expressions.aggregate.BoolOr | bool_or | SELECT bool_or(col) FROM VALUES (true), (false), (false) AS tab(col) | struct<bool_or(col):boolean> |
-| org.apache.spark.sql.catalyst.expressions.aggregate.BoolOr | some | SELECT some(col) FROM VALUES (true), (false), (false) AS tab(col) | struct<some(col):boolean> |
+| org.apache.spark.sql.catalyst.expressions.aggregate.BoolAnd | bool_and | SELECT bool_and(col) FROM VALUES (true), (true), (true) AS tab(col) | struct<min(col):boolean> |
Review comment:
Do you know how we introduce this change?
--
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