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

feat(frontend): Acosd function #9876

Merged
merged 20 commits into from
Jul 26, 2024
Merged

feat(frontend): Acosd function #9876

merged 20 commits into from
Jul 26, 2024

Conversation

CAJan93
Copy link
Contributor

@CAJan93 CAJan93 commented May 17, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Implement the Acosd function. This implementation is slightly different from the PSQL implementation. I do not return an error on acosd(Inf), but instead return NaN. Also returns NaN when inout is out of range.

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

  • I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

  • My PR DOES NOT contain user-facing changes.
Click here for Documentation

Types of user-facing changes

acosd trigonometric function implementation.

Release note

Add acosd function, which calculates the inverse acos in degrees.

@CAJan93 CAJan93 changed the title featAcosd feat(frontend): Acosd function May 17, 2023
@CAJan93 CAJan93 added the user-facing-changes Contains changes that are visible to users label May 18, 2023
@CAJan93 CAJan93 enabled auto-merge May 18, 2023 12:00
@github-actions
Copy link
Contributor

This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review.

auto-merge was automatically disabled January 31, 2024 06:35

Merge queue setting changed

@fuyufjh fuyufjh closed this Jun 14, 2024
@CAJan93 CAJan93 reopened this Jun 27, 2024
@CAJan93 CAJan93 requested a review from a team as a code owner June 27, 2024 07:17
Copy link

gitguardian bot commented Jun 27, 2024

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9425213 Triggered Generic Password a271c3c ci/scripts/e2e-source-test.sh View secret
9425213 Triggered Generic Password a271c3c ci/scripts/e2e-sink-test.sh View secret
9425213 Triggered Generic Password a271c3c ci/scripts/e2e-source-test.sh View secret
9425213 Triggered Generic Password a271c3c ci/docker-compose.yml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@CAJan93
Copy link
Contributor Author

CAJan93 commented Jun 27, 2024

@xiangjinwu Sorry, I completely forgot about this one. Does this LGTY?

@CAJan93 CAJan93 requested review from xiangjinwu and removed request for a team June 27, 2024 13:05
@xiangjinwu
Copy link
Contributor

@xiangjinwu Sorry, I completely forgot about this one. Does this LGTY?

No worries. We received no requests from customers needing trigonometric functions 😂

@CAJan93
Copy link
Contributor Author

CAJan93 commented Jun 27, 2024

No worries. We received no requests from customers needing trigonometric functions 😂

Guess so 😂 Was still nice to work on the kernel side for a while.

@CAJan93
Copy link
Contributor Author

CAJan93 commented Jun 28, 2024

No idea why we get these imprecisions. Doing everything just like the PG implementation. Maybe it is a difference between Rust and C?

For now I will hardcode these cases. I feel like it is not wort putting more energy into this.

SELECT x,
       tand(x),
       tand(x) IN ('-Infinity'::float8,-1,0,
                   1,'Infinity'::float8) AS tand_exact,
       cotd(x),
       cotd(x) IN ('-Infinity'::float8,-1,0,
                   1,'Infinity'::float8) AS cotd_exact
FROM (VALUES (0), (45), (90), (135), (180),
      (225), (270), (315), (360)) AS t(x);
2024-06-27T17:28:44.720818Z ERROR risingwave_regress_test::schedule: Diff:
-  x  |   tand    | tand_exact |   cotd    | cotd_exact
------+-----------+------------+-----------+------------
-   0 |         0 | t          |  Infinity | t
-  45 |         1 | t          |         1 | t
-  90 |  Infinity | t          |         0 | t
- 135 |        -1 | t          |        -1 | t
- 180 |         0 | t          | -Infinity | t
- 225 |         1 | t          |         1 | t
- 270 | -Infinity | t          |         0 | t
- 315 |        -1 | t          |        -1 | t
- 360 |         0 | t          |  Infinity | t
+  x  |        tand         | tand_exact |        cotd         | cotd_exact
+-----+---------------------+------------+---------------------+------------
+   0 |                   0 | t          |            Infinity | t
+  45 |  1.0000000000000002 | f          |  0.9999999999999998 | f
+  90 |            Infinity | t          |                   0 | t
+ 135 | -1.0000000000000002 | f          | -0.9999999999999998 | f
+ 180 |                   0 | t          |           -Infinity | t
+ 225 |  1.0000000000000002 | f          |  0.9999999999999998 | f
+ 270 |           -Infinity | t          |                   0 | t
+ 315 | -1.0000000000000002 | f          | -0.9999999999999998 | f
+ 360 |                   0 | t          |            Infinity | t

@CAJan93
Copy link
Contributor Author

CAJan93 commented Jul 1, 2024

Shall we merge it @xiangjinwu ? 😁 If we really don't need the function at the moment I'd suggest to close the PR.

@xiangjinwu
Copy link
Contributor

Shall we merge it @xiangjinwu ? 😁 If we really don't need the function at the moment I'd suggest to close the PR.

I am looking at several avro issues recently 🥵

@CAJan93
Copy link
Contributor Author

CAJan93 commented Jul 2, 2024

I am looking at several avro issues recently 🥵

No worries. I think this is low priority 😁

@graphite-app graphite-app bot requested a review from xxchan July 26, 2024 02:51
@xxchan xxchan removed their request for review July 26, 2024 03:12
@xxchan xxchan added this pull request to the merge queue Jul 26, 2024
Merged via the queue into main with commit 849bed0 Jul 26, 2024
31 of 32 checks passed
@xxchan xxchan deleted the acosd branch July 26, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants