hhr293 commented on PR #58424: URL: https://github.com/apache/spark/pull/58424#issuecomment-5666217511
Hi, @LuciferYang @cloud-fan @sunchao @peter-toth Thanks for the latest review. I’ve pushed another update addressing the new comments. The main changes are: * added a Q95-shaped regression test covering both Pattern A' and A2; * strengthened the negative tests so they actually reach the intended guards; * extended the clock-dependent timestamp guard/tests to cover nested STRING/VARIANT -> TIMESTAMP_LTZ cases, with NTZ positive controls; * added coverage for the bare A2 path with right-side ExprId remapping; * tightened the string safety check to `supportsBinaryOrdering` and clarified the MIN/MAX type-safety comments. I also added a separate performance probe for variable-width inequality columns. In the ~1 GB forced-SMJ case, the speedups (OFF / ON) for `m=1/3/5` are: `INT`: 0.8x / 1.5x / 1.9x `STRING`: 0.6x / 0.9x / 1.2x `BINARY`: 0.8x / 1.1x / 1.5x As expected, `STRING`/`BINARY` have a higher break-even point because the rewrite uses `SortAggregateExec`, but they are not categorically unprofitable. This is also why I kept profitability separate from the correctness type gate and left the rule opt-in. The targeted suite and scalastyle pass with these changes. Could you please take another look? 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]
