Raviston6296 commented on PR #60117: URL: https://github.com/apache/spark/pull/60117#issuecomment-6652694763
Thanks for the detailed write-up. I went through it independently. Here is what I checked, plus two questions. Checked: - `PushPredicateThroughNonJoin` (`isExpensive`, and `expensivePartsOf` in the projection split) is the only optimizer code that reads `Expression.expensive`. So handling the setting in `isExpensive` covers every planning effect of SPARK-59693, apart from the split's sharing analysis already raised above, which only changes plan shape. - SPARK-59693 is only on `master` and `branch-4.x`. It is not on `branch-4.3` or `branch-4.2`, and it is not in any release tag, so a static `false` for a missing key is right for every released checkpoint. I assume this goes to `branch-4.x` too. - With the setting off, `transformUp` rewrites inner UDFs first, so nested Python UDFs (`py(py(a))`) and a Python UDF over an expensive argument (`py(expensiveRLike(...))`) are classified as in 4.3. `PrettyPythonUDF` is expensive only through `hasExpensiveChild`, and it is not foldable, so an RLIKE pattern produced by a UDF stays expensive. - I agree with the inline `!e.expensive` short-circuit. The stand-in can only lower `expensive`, so a cheap `e` needs no rewrite. Questions: 1. In the testing table, Issue 2 gives count 1 for "this PR -> this PR" and count 2 for 4.3.0. So a new query of that shape on 4.4 drops a row that 4.3 kept, because the filter now stays above `EventTimeWatermark`. That follows from SPARK-59693 and seems acceptable, but since the difference is visible without any restart, would a line in the migration guide help? 2. Slightly outside this PR: SPARK-60602 is on `branch-4.3` and `branch-4.2` but in no release yet, so it will first ship in 4.2.2/4.3.1. When an expensive conjunct is written before a throwable one (e.g. `assert_true`), it keeps the throwable conjunct above the projection, where 4.3.0 pushed it down. If such a filter sits over a stream-stream join, could a restart from a 4.3.0 checkpoint on 4.3.1 hit the same `STATE_STORE_VALUE_SCHEMA_NOT_COMPATIBLE` as Issue 1? It needs a throwable conjunct, so it is much rarer, and there is no checkpoint pin for it. I have not reproduced it; I'm just raising it since the mechanism looks the same. -- 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]
