peter-toth commented on code in PR #60038:
URL: https://github.com/apache/spark/pull/60038#discussion_r4452809202
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -55,7 +55,10 @@ object ConstantFolding extends Rule[LogicalPlan] {
private def hasNoSideEffect(e: Expression): Boolean = e match {
case _: Attribute => true
case _: Literal => true
- case c: Cast if !conf.ansiEnabled => hasNoSideEffect(c.child)
+ // A cast keeps the eval mode it was analyzed with (for example, in a
view), so check it as
+ // well. Keep the session check: some LEGACY casts can still fail, such as
an interval
+ // overflow, and this should not start folding them in ANSI sessions.
+ case c: Cast if !conf.ansiEnabled && c.evalMode != EvalMode.ANSI =>
hasNoSideEffect(c.child)
Review Comment:
**Finding 1.** The session check stops a failing LEGACY cast from being
folded in an ANSI session. In a non-ANSI session, the same cast is still folded
and its error is lost. I ran this on base and on this head, with the same
result on both:
```scala
val i = AttributeReference("i",
DayTimeIntervalType(DayTimeIntervalType.SECOND))()
withSQLConf(SQLConf.ANSI_ENABLED.key -> "false") {
val e = Size(CreateArray(Seq(Cast(i, ByteType, evalMode =
EvalMode.LEGACY))))
ConstantFolding.constantFolding(e) // Literal(1)
// throws CAST_OVERFLOW
BindReferences.bindReference(e, Seq(i)).eval(InternalRow(1000L * 1000000L))
}
```
This has been there since SPARK-37392 and is outside this PR's scope. Could
you file a ticket? A fix could fold a LEGACY cast only when its source and
target types can't fail in LEGACY mode. I didn't find a helper for that in
`Cast`, and haven't tried one.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:
##########
@@ -55,7 +55,10 @@ object ConstantFolding extends Rule[LogicalPlan] {
private def hasNoSideEffect(e: Expression): Boolean = e match {
case _: Attribute => true
case _: Literal => true
- case c: Cast if !conf.ansiEnabled => hasNoSideEffect(c.child)
+ // A cast keeps the eval mode it was analyzed with (for example, in a
view), so check it as
+ // well. Keep the session check: some LEGACY casts can still fail, such as
an interval
+ // overflow, and this should not start folding them in ANSI sessions.
+ case c: Cast if !conf.ansiEnabled && c.evalMode != EvalMode.ANSI =>
hasNoSideEffect(c.child)
case _: NoThrow if e.deterministic => e.children.forall(hasNoSideEffect)
Review Comment:
**Finding 2.** `CreateMap` is marked `NoThrow`
(`complexTypeCreator.scala:202`), but evaluating it can throw on a null key or
a duplicate key. The `Size(c: CreateMap, _)` fold at `expressions.scala:95`
also counts every key, even when two keys are equal at runtime. I ran these on
base and on this head, with the same result on both:
| Expression | Folded | Evaluated |
|---|---|---|
| `size(map(a, 1, a, 2))`, `EXCEPTION` | `2` | throws `DUPLICATED_MAP_KEY` |
| `size(map(a, 1, a, 2))`, `LAST_WIN` | `2` | `1` |
| `size(map(a, 1))`, `a` is NULL | `1` | throws `NULL_MAP_KEY` |
The `LAST_WIN` row is a wrong result, not just a lost error. It has been
there since SPARK-33544.
[SPARK-59604](https://issues.apache.org/jira/browse/SPARK-59604) is the same
`LAST_WIN` problem in `SimplifyExtractValueOps`, but it doesn't cover this
fold. Could you file a ticket for this one too? A fix could drop `NoThrow` from
`CreateMap`, and fold its size only when the keys are distinct non-null
literals. I haven't tried 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
