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

DMN 1.4 - 1141-feel-round-up-function #592

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

StrayAlien
Copy link
Contributor

No description provided.

@opatrascoiu
Copy link
Contributor

@StrayAlien Thank you for the tests.

Before we start reviewing the rounding tests here is the background of the proposal:

  • the goal of the new round functions I submitted to RTF was to have support for all the available rounding modes. Prior to the proposal DMN already had support for certain rounding modes, see decimal(), floor(), and ceiling() functions.
  • the semantics is identical to https://docs.oracle.com/javase/8/docs/api/java/math/RoundingMode.html. If the spec does not cover all cases (e.g. negative scales) we should follow the one above and add examples / working in spec.

AFAIK there are already test cases for decimal() that cover some edge cases (e.g. decimal(1/3, 2.5) where the expected behavior is to use only the int value of the scale). We need to align the new tests with the existing ones to treat the edge cases in the same way (e.g. negative scales).

Most of the test cases look good. So far, I spotted a few tests that need to be discussed: 0015, 0016_a, 0016_b, 0017_b. Please review them based on the above.

Thank you again.

@StrayAlien
Copy link
Contributor Author

Hi @opatrascoiu - thanks for that link and the comments. Yes, let's discuss those tests for sure.

As there are quite a number of DMN 1.4 suites that dealing with scales, I'll open up a discussion so we have it all in a single place.

here: #607

@baldimir
Copy link
Collaborator

The part where the scale of -6111 is mentioned in the spec is "Table 76: Semantics of numeric functions", after the table. There is written "Scale is in the range [−6111 ..6176]". We need to clarify with RTF, why these numbers.

@opatrascoiu
Copy link
Contributor

@baldimir I raised two issues with RTF to discuss. See #607

@baldimir
Copy link
Collaborator

baldimir commented Aug 9, 2023

Thank you @opatrascoiu.

@opatrascoiu
Copy link
Contributor

The issue with the "Scale is in the range [−6111 ..6176]" has been clarified in RTF. The tests look good to me.

@SimonRinguette
Copy link
Contributor

I would love to remove test case 006 and have the RTF add a version of the function without scale to align with ceiling and floor functions!

@baldimir
Copy link
Collaborator

After discussion, there was not an agreement if we want the test 006 in or not, so because of this, we should comment it out with a description why we did so.

@SimonRinguette
Copy link
Contributor

RTF issue: https://issues.omg.org/issues/INBOX-1769

@StrayAlien
Copy link
Contributor Author

test 6 commented out as per request

@SimonRinguette
Copy link
Contributor

The 11b test that call using a named parameter (only one) should have also been removed with test 6 because it is the same test but with named parameter

@StrayAlien
Copy link
Contributor Author

11b commented out

@baldimir baldimir merged commit b154a89 into dmn-tck:master Nov 16, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants