Skip to content
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 function overload LOG(base, x) for logarithm of x to base #5245

Merged
merged 4 commits into from
Feb 12, 2023

Conversation

comphead
Copy link
Contributor

Which issue does this PR close?

Closes #5206 .

Rationale for this change

Add SQL function overload LOG(base, x) for logarithm of x to base

What changes are included in this PR?

Are these changes tested?

Test included

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Feb 10, 2023
@comphead
Copy link
Contributor Author

@stuartcarnie @alamb with log function we have some discrepancy with PG, its not brought up with this PR, it existed before

❯ select log(1, 64), log(0);
+-------------------------+---------------+
| log(Int64(1),Int64(64)) | log(Int64(0)) |
+-------------------------+---------------+
| inf                     | -inf          |
+-------------------------+---------------+

whereas PostgresSQL doesn't allow queries like that and fail the query

@alamb
Copy link
Contributor

alamb commented Feb 11, 2023

whereas PostgresSQL doesn't allow queries like that and fail the query

For anyone else who is curious, here is what postgres does with this query:

postgres=# select log(0);
ERROR:  cannot take logarithm of zero
postgres=# select log(1, 64);
ERROR:  division by zero

@alamb
Copy link
Contributor

alamb commented Feb 11, 2023

Thank you very much for this contribution @comphead

@comphead
Copy link
Contributor Author

whereas PostgresSQL doesn't allow queries like that and fail the query

For anyone else who is curious, here is what postgres does with this query:

postgres=# select log(0);
ERROR:  cannot take logarithm of zero
postgres=# select log(1, 64);
ERROR:  division by zero

@alamb would you like to solve edgecases as part of this PR or another one?

@alamb
Copy link
Contributor

alamb commented Feb 11, 2023

@alamb would you like to solve edgecases as part of this PR or another one?

A different one, please

# under the License.

#############
## Scalar Tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Scalar Tests
## Scalar Function Tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# log scalar nulls 2
query IT rowsort
select log(null, null) a, log(null) b
----
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add the test cases you identified in the PR here (log(0), log(1, 64)) as well to document the behavior and also file a ticket to document the discrepancy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, filed #5259

@alamb
Copy link
Contributor

alamb commented Feb 11, 2023

Thanks @comphead -- this looks really great. cc @stuartcarnie

@stuartcarnie
Copy link
Contributor

stuartcarnie commented Feb 11, 2023

Looks great, thanks @comphead 🙏

I tried to look at the source of PostgreSQL, to see the implementation of their log function, but can't search on my phone 😏

@alamb alamb merged commit 8a609dc into apache:master Feb 12, 2023
@alamb
Copy link
Contributor

alamb commented Feb 12, 2023

Thanks again @comphead

@ursabot
Copy link

ursabot commented Feb 12, 2023

Benchmark runs are scheduled for baseline = 0f95966 and contender = 8a609dc. 8a609dc is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SQL function overload LOG(base, x) for logarithm of x to base
4 participants