jayzhan211 commented on PR #24519: URL: https://github.com/apache/datafusion/pull/24519#issuecomment-5377907840
> Thank you! This PR does 1) perf 2) fix float +0.0 and -0.0 normalization, right? I have some thoughts on the zero normalization part: > > Is it possible to add an optimizer rule, to rewrite the expression like `match_condition t1.v1 < t2.v1` --> `match_condition normalize_zero(t1.v1) < normalize_zero(t2.v1)` , and this way we can move the complexity outside the operators. > > Other implementations seem can also benefit: #22835 I have concerns for moving this out as optimizer rule 1. The match expression is also the sort key. AsOfJoinExec requires both inputs to be sorted on the match expression. If we rewrite it to `normalize_zero(v1)`, the required ordering becomes "sorted by normalize_zero(v1)", and the planner no longer recognizes an input already sorted by v1 as satisfying it — so we'd insert an extra sort on both sides. And that sort would be pointless: -0.0 already sorts right next to +0.0, so normalizing never breaks sortedness in the first place. 2. Correctness shouldn't depend on an optimizer rule running. AsOfJoinExec::try_new is public and used directly by tests and custom planners — those would silently produce wrong results if the rule didn't run. Optimizer rule is optional; correctness belongs in the operator. -- 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]
