dongjoon-hyun commented on PR #60162: URL: https://github.com/apache/spark/pull/60162#issuecomment-6692025167
Thank you, @peter-toth, for the thorough review. Most of the findings were reproduced, and several of them (5, 6, 7 and 8) were real bugs or test failures that I had missed. Here is what changed in d74dfff42aa and in the PR description. **Fixed** - Finding 5: the SPARK-58792 test in `HiveUDFSuite` runs with `spark.sql.legacy.inferFailableFiltersFromConstraints` set to `true`, because a Hive UDF is only copied with it. - Finding 6: the new configuration declares `ConfigBindingPolicy.NOT_APPLICABLE`. - Finding 7: `canBeFilter` now walks the expression top-down. It judges an `InSubquery` by its values and its `joinCond`, an `Exists` by its `joinCond`, and treats a `ScalarSubquery` as a constant only when it is not correlated. A new `DataFrameSuite` test runs the correlated `IN` in the `ON` clause of a `LEFT JOIN`. - Finding 8: because of the top-down walk, each cast is judged by the type of its original input. `CAST(CAST(y AS FLOAT) AS INT)` is covered in `DataFrameSuite`. - Finding 9: the assertion for a literal binding that makes the other side a failing constant is added. It fails with `DIVIDE_BY_ZERO` when `canBeFilter` folds with `isConditionalBranch = false`. - Finding 2: the tests pin ANSI mode (`Divide(..., EvalMode.ANSI)` and `withSQLConf` for the narrowing cast), so they no longer depend on the session mode. **Kept as it is, with the documentation corrected** - Finding 1: this PR stays independent of #60119, so the filters in your table are not inferred until `canEvaluateUnconditionally` accepts them. The migration guide, the configuration doc and the PR description now say that the check is conservative and also leaves out some expressions that cannot fail, such as `LIKE 'a%'`, `year(d) = 2024` and ANSI-off arithmetic, and that the legacy configuration restores the previous behavior. **Not in this PR** - Finding 4 (a TRY copy with `IS NOT FALSE`): left out of this PR for the reasons in my reply. - Finding 3 (`PushExtraPredicateThroughJoin`): confirmed with the same result, and it will get its own JIRA. I did not add a test that pins the semi-join of a correlated `EXISTS` in the `ON` clause (second half of finding 7). It only changes the plan, not the rows, and I could not observe the difference in a query result. Verification: `InferFiltersFromConstraintsSuite`, `ConstraintPropagationSuite`, `FilterPushdownSuite`, the new `DataFrameSuite` tests, `*PlanStability*Suite` (golden files unchanged), `HiveUDFSuite` (SPARK-58792) and `SparkConfigBindingPolicySuite` pass, and scalastyle has no errors. -- 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]
