-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-32931][SQL] Unevaluable Expressions are not Foldable #29798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -327,7 +327,6 @@ case class InSubquery(values: Seq[Expression], query: ListQuery) | |
|
|
||
| override def children: Seq[Expression] = values :+ query | ||
| override def nullable: Boolean = children.exists(_.nullable) | ||
| override def foldable: Boolean = children.forall(_.foldable) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have some more confirmation of why it was written this way originally, and whether or not we're losing anything by just making
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems there was no specific reason: #21403 . We just followed It's obviously not foldable as it needs to evaluate a subquery.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK +1 with @cloud-fan 's comment. |
||
| override def toString: String = s"$value IN ($query)" | ||
| override def sql: String = s"(${value.sql} IN (${query.sql}))" | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,12 +45,22 @@ case class WindowSpecDefinition( | |
|
|
||
| override def children: Seq[Expression] = partitionSpec ++ orderSpec :+ frameSpecification | ||
|
|
||
| /* | ||
| * The result of an OffsetWindowFunction is dependent on the frame in which the | ||
|
||
| * OffsetWindowFunction is executed, the input expression and the default expression. Even when | ||
| * both the input and the default expression are foldable, the result is still not foldable due to | ||
| * the frame. | ||
| * | ||
| * Note, the value of foldable is set to false in the trait Unevaluable | ||
| * | ||
| * override def foldable: Boolean = false | ||
| */ | ||
|
|
||
| override lazy val resolved: Boolean = | ||
| childrenResolved && checkInputDataTypes().isSuccess && | ||
| frameSpecification.isInstanceOf[SpecifiedWindowFrame] | ||
|
|
||
| override def nullable: Boolean = true | ||
| override def foldable: Boolean = false | ||
| override def dataType: DataType = throw new UnsupportedOperationException("dataType") | ||
|
|
||
| override def checkInputDataTypes(): TypeCheckResult = { | ||
|
|
@@ -144,7 +154,6 @@ case object RangeFrame extends FrameType { | |
| sealed trait SpecialFrameBoundary extends Expression with Unevaluable { | ||
| override def children: Seq[Expression] = Nil | ||
| override def dataType: DataType = NullType | ||
| override def foldable: Boolean = false | ||
| override def nullable: Boolean = false | ||
| } | ||
|
|
||
|
|
@@ -168,7 +177,6 @@ case object CurrentRow extends SpecialFrameBoundary { | |
| sealed trait WindowFrame extends Expression with Unevaluable { | ||
| override def children: Seq[Expression] = Nil | ||
| override def dataType: DataType = throw new UnsupportedOperationException("dataType") | ||
| override def foldable: Boolean = false | ||
| override def nullable: Boolean = false | ||
| } | ||
|
|
||
|
|
@@ -275,7 +283,6 @@ case class UnresolvedWindowExpression( | |
| windowSpec: WindowSpecReference) extends UnaryExpression with Unevaluable { | ||
|
|
||
| override def dataType: DataType = throw new UnresolvedException(this, "dataType") | ||
| override def foldable: Boolean = throw new UnresolvedException(this, "foldable") | ||
| override def nullable: Boolean = throw new UnresolvedException(this, "nullable") | ||
| override lazy val resolved = false | ||
| } | ||
|
|
@@ -287,7 +294,6 @@ case class WindowExpression( | |
| override def children: Seq[Expression] = windowFunction :: windowSpec :: Nil | ||
|
|
||
| override def dataType: DataType = windowFunction.dataType | ||
| override def foldable: Boolean = windowFunction.foldable | ||
| override def nullable: Boolean = windowFunction.nullable | ||
|
|
||
| override def toString: String = s"$windowFunction $windowSpec" | ||
|
|
@@ -365,14 +371,6 @@ abstract class OffsetWindowFunction | |
|
|
||
| override def children: Seq[Expression] = Seq(input, offset, default) | ||
|
|
||
| /* | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep the original comment and just add a new line of comment that says
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just added it back |
||
| * The result of an OffsetWindowFunction is dependent on the frame in which the | ||
| * OffsetWindowFunction is executed, the input expression and the default expression. Even when | ||
| * both the input and the default expression are foldable, the result is still not foldable due to | ||
| * the frame. | ||
| */ | ||
| override def foldable: Boolean = false | ||
|
|
||
| override def nullable: Boolean = default == null || default.nullable || input.nullable | ||
|
|
||
| override lazy val frame: WindowFrame = { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not what you'd actually want.
I'm actually really curious why
RuntimeReplaceablehas to implementUnevaluablein the first place. It doesn't really make sense. We could just turnRuntimeReplaceableinto a proper wrapper that delegates everything, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is noop in practice, because we replace
RuntimeReplaceableat the beginning of the optimizer, so foldable or not doesn't really matter.Semantically, it might be clearer to just make
RuntimeReplaceablea pure delegator likeAlias, but it complicates the expression tree. Anyway, this is out of the scope of this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it complicate the expression tree? There's a
TaggingExpressiontrait (https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraintExpressions.scala#L24) that'd handle this perfectly. MakingRuntimeReplaceableimplementTaggingExpressioninstead ofUnevaluablemakes much more sense to me. It'll still retain the same semantics of being replaced early.I agree that we can do it in a separate PR, but I'd really like to make sure the refactoring done while we're at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah
TaggingExpressionsounds like a good idea, we can try it later.