-
Notifications
You must be signed in to change notification settings - Fork 839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sql-compliant feature for enabling sql-compliant kernel behavior #2568
Comments
There is an IEEE standard for total ordering of floats, I wonder if we could use that? Aside from being a standard, where I've not managed to find an authoritative SQL standard for floats, it can be implemented with relatively cheap bit manipulation, instead of more expensive branching. There is a built-in implementation here, which also does a good job explaining its behaviour. Theoretically the performance may be good enough that we can just always have this behaviour without needing a feature flag? What do you think? Also tagging @alamb |
Hmm, the NaN ordering of the IEEE standard treats NaN value larger than any other numbers. Looks it is consistent with the SQL-compliant ordering we need. I can rewrite these kernels with total_cmp. As without a flag, it is somehow breaking existing behavior but I think current behavior can be thought as wrong or not useful. |
I think |
Can you provide a more specific example for |
We've discussed this in PRs. It depends on SQL implementations. As far as I know, Spark and postgresql treats NaNs equal. |
I took a look too, and yes it looks like |
Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Some kernels behaves different with SQL semantics. For example, by definition, NaN is not equal to itself but NaN is equal to NaN with SQL semantics. Using current comparison kernels in SQL system leads to different behavior and generates incorrect results.
Describe the solution you'd like
We should provide SQL-compliant kernels which can be enabled by feature flag.
Describe alternatives you've considered
Additional context
The text was updated successfully, but these errors were encountered: