hhr293 commented on PR #58424: URL: https://github.com/apache/spark/pull/58424#issuecomment-6164707115
> Re-reviewed unchanged head `809dde3c13bca7d06b9991f73705dab9189c33cc` with five independent reviewers. I read the replies acknowledging both findings and revalidated the complete change. No additional confirmed defect was found. > > Both existing P2 findings remain reproducible on this head: > > * [Nested complex-to-STRING conversion](https://github.com/apache/spark/pull/58424#discussion_r4129255599): ARRAY/MAP/STRUCT container conversions still bypass the guard. > * [Floating arithmetic equivalence](https://github.com/apache/spark/pull/58424#discussion_r4129255604): reassociated DOUBLE additions beneath an INT cast still pass `sameResult` despite different values. > > Fresh runs return key `1` with the rewrite off and no rows with it on in both cases, with whole-stage codegen disabled and enabled; the matching controls remain correct. The proposed recursive source/target checks and conservative arithmetic guard are appropriate directions, but those changes are not yet present at this head. I have kept the existing inline threads instead of duplicating them. > > Additional validation this round: I compiled the current rule and the current `RewriteSelfJoinInequalityToAggregateSuite` and ran all 41 tests successfully. The separate reproduction harness ran 32 query executions. These local runs used current-source overlays on the prior-head CI assembly/test dependencies, not a clean full Spark build. Current-head CI also records 41 passing suite tests, with 29 successful checks and one skipped Kubernetes check. Full and last-commit whitespace checks passed. > > I am holding approval until the two wrong-result cases are fixed. Thanks for the thorough review and for identifying these two correctness issues. I've filed separate JIRAs and submitted fixes for both at the Catalyst level: - SPARK-60179 / [PR #59394](https://github.com/apache/spark/pull/59394): Fixes nested complex-to-STRING casts being incorrectly classified as time-zone independent. - SPARK-60178 / [PR #59437](https://github.com/apache/spark/pull/59437): Prevents floating-point reassociation during canonicalization, which can cause incorrect sameResult comparisons. Both issues affect Spark's shared Catalyst semantics beyond this rewrite, including cache reuse and expression equivalence. I believe fixing them at the source is preferable to adding rule-specific workarounds here. Once both fixes are available, I'll revalidate the two reported reproductions with the rewrite enabled and disabled. -- 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]
