ozankabak commented on PR #11584: URL: https://github.com/apache/datafusion/pull/11584#issuecomment-2245191841
Apart from @berkaysynnada's outstanding suggestions the only thing I see is that we would need to use two different π values, depending on whether it is in the lower bound or the upper bound of the resulting interval. If π will be in the upper bound, we should use the smallest floating point number that is greater than the mathematical (precise) value of π. Let's call this `pi_upper`. Conversely, if π will be in the lower bound, we should use the largest floating point number that is less than the mathematical (precise) value of π. Since π is not exactly representable, the `pi` you get from Rust is actually either `pi_upper` or `pi_lower` (you need to check). You can obtain the other one by using the `nextafter` function (let's just be careful to do this only once and use it as a constant everywhere). Apart from this detail that is addressable by a small fix, this seems to be a great PR. Thank you. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
