You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2023/02/12 07:13:59 UTC
[GitHub] [arrow-datafusion] comphead opened a new issue, #5259: Make `log` function to be in sync with PostgresSql
comphead opened a new issue, #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
The `log` function has some discrepancy with Postgres for edgecases e.g.
```
❯ select log(1, 64), log(0);
+-------------------------+---------------+
| log(Int64(1),Int64(64)) | log(Int64(0)) |
+-------------------------+---------------+
| inf | -inf |
+-------------------------+---------------+
```
The query executes ok, but in PostgresSql the query fail fast
```
postgres=# select log(0);
ERROR: cannot take logarithm of zero
postgres=# select log(1, 64);
ERROR: division by zero
```
**Describe the solution you'd like**
Investigate and implement log function to be the same behavior as for PostgresSql for edgecases above and perhaps for other like negative numbers, etc
**Describe alternatives you've considered**
Not doing this
**Additional context**
Found when working on https://github.com/apache/arrow-datafusion/pull/5245
--
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-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1427178897
Other edgecases
```
PG log(-1), log(-10, 1), log(10, -1)
cannot take logarithm of a negative number
DF
+----------------+--------------------------+--------------------------+
| log(Int64(-1)) | log(Int64(-1),Int64(10)) | log(Int64(1),Int64(-10)) |
+----------------+--------------------------+--------------------------+
| NaN | NaN | NaN |
+----------------+--------------------------+--------------------------+
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1432306444
@ozankabak thanks for feedback, the discussion is already much broader than log function itself.
You may want to create a separate topic to discuss failfast/failsafe approaches in detail. Moreover this behavior has to be consistent across all math functions, and probably cast functions as well.
I just noticed, since we use rust math functions, it returns `NaN` values for outlier scenarios, and this going to be consistent, even not complying to how PG works
```
❯ select sqrt(-1);
+-----------------+
| sqrt(Int64(-1)) |
+-----------------+
| NaN |
+-----------------+
```
whereas PG bursts as
```
cannot take square root of a negative number
```
I'll leave it for @alamb @andygrove to decide
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1433613447
> Probably its worth to document somewhere
Agree -- the best place would be https://github.com/apache/arrow-datafusion/blob/main/docs/source/user-guide/expressions.md perhaps. Do either of you have some time to write one? Otherwise I will do so
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1432363234
Yes, Rust is following the usual numerics convention (which is unsurprising). I agree that there has to be a general config through which the behavior applies consistently.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1427311934
This behavior doesn't seem unreasonable to me. IIRC, it agrees with IEEE 754. Also, do we want to error out when this happens in the middle of a large table (or worse, an unbounded stream)? This is a local out-of-domain issue, and the output value indicates that. Having worked on numerics for a long time in a wide variety of contexts, Datafusion's behaviors seems more reasonable to me.
On the other hand, we want PG compatibility in general. Maybe we can add a configuration flag that controls whether we bail out when these issues occur, or return a special value.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1433534237
> If we want to support PG-like behavior through a general configuration option, we will need to write wrappers around these functions that consult the config.
I agree the only way everyone will be satisfied is with a config that controls it.
I suspect this is a non trivial amount of effort to implement this consistently - so we should be aware of that as we start down the road
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1429133926
As I said, I see the value in failing fast but doing that unconditionally is simply not an option in certain applications when you are executing a long-running query on a stream. That's why I suggested to have a configuration option to control this. Having both behaviors available allows us to cater to all use cases without impacting anyone badly, and has a nice side-effect of obviating the need for long philosophical discussions about failing fast vs. special values.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1433629497
Filed https://github.com/apache/arrow-datafusion/issues/5312
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb closed issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #5259: Make `log` function to be in sync with PostgresSql
URL: https://github.com/apache/arrow-datafusion/issues/5259
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1433552521
Agreed
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1433553309
Thanks @alamb so we can close this ticket then and leave all math scalar functions to work in sync with rust math functions and out of sync with PG. Probably its worth to document somewhere
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on issue #5259: Make `log` function to be in sync with PostgresSql
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #5259:
URL: https://github.com/apache/arrow-datafusion/issues/5259#issuecomment-1428974364
@ozankabak Its probably point of long disputes what is better: failfast or return null/nan etc.
I'm still thinking we should stick to PG behavior, as it is consistent and expectable from users migrating their queries.
From personal point of view it seems failfast is better solution anyway as it doesn't silently corrupt the data and give the user responsibility how to deal with bad data on his side. Math function has to be simple and support only supportable data for given function.
Other day old spark versions turned the value into null if it cannot be casted to timestamp and this was sooooo frustrating, that spark went away and now the `to_timestamp` function fails fast.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org