LuciferYang commented on PR #58045: URL: https://github.com/apache/spark/pull/58045#issuecomment-5405711425
> We should be careful when we change the expression evaluation order or invocation times. Do the existing expression properties sufficient to make safe judgement here? e.g. stateful, throwable, deterministic, etc. They are not sufficient, and investigating that is what changed this PR. It no longer pre-evaluates anything, so there is no such judgement left to make. On the properties: the bug set coincides with `stateful`. `spark_partition_id()` inlined is still correct, while `monotonically_increasing_id()` returns 6 `true` rows where 3 is right, so excluding stateful expressions from pre-evaluation would have left the PR empty. `throwable` is not available as a property either. I tried deciding it from the expression, and what that admits is too narrow to cover the bug: `rand() / col` and `randstr(3, 0)` kept the wrong result. What the PR does now is make `With` evaluable and memoize each common expression for the row being evaluated. Nothing is relocated: a definition is evaluated where the child would have evaluated it, so an expression that raises raises on exactly the rows it did before, and a generator advances on exactly the rows that reach a reference. Invocation times change in one direction only, from once per reference to once. A reference's domain is in fact narrower than its branch's — `BETWEEN` builds an `And`, and the second reference is not read on a row whose first comparison is false, which pre-evaluation cannot express. If this approach looks acceptable, I will file a follow-up for the two shapes that are still substituted: a common expression in a join condition that references both sides, and one whose definition cannot be put in a `Project`. Could you take another look when you have time? Thanks. -- 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]
