apache / incubator-kie-issues

Apache License 2.0
12 stars 1 forks source link

Accumulator functions on FEEL are not compatible with lists of durations #977

Open tiagobento opened 7 months ago

tiagobento commented 7 months ago

This issue has come to my attention first reported by @timwuthenow, while trying to add a list of days and time duration using the sum function.

To my surprise, it indeed didn't work, and looking as the specification (thanks @baldimir and @gitgabrio) there's indeed no mention to types other than number being used with the sum function.

image

It sounds odd that the sum function wouldn't be compatible with types that do work with the + operator.

Also, there was no way to work around this limitation, making it impossible to add a dynamic list of date and time duration.

My opinion is that, although not part of the specification, the sum function should work with lists of all types that the + operator supports, in the non-strictly-compatible mode of the DMN engine. (Thanks @baldimir for mentioning that this exists!). This of course would apply to other accumulator functions present on the FEEL specification:


Also, as curiosity, does anyone know why wouldn't FEEL implement a generic reduce function? I mean, having those accumulator functions is great, of course, but not exposing the raw accumulator technique might hurt users who need a little bit more control, no?

timwuthenow commented 7 months ago

Thanks @tiagobento, just for context of why from a business point of view this matters from an execution point of view is the following scenario: I have a worker who I am tracking their total hours worked in a certain role, if they exceed a threshold, we need to know. So the accumulator would look at all of the duration times in the collection (e.g. Monday PT2H5M, Tuesday PT3H30M, etc) and be able to total those up. This is a very real and common situation within the engine and something that I would have thought would have been captured.

The expected behavior of the above would be that its at least 5 hours and 35 minutes, but currently I think we return null in the return (at least when checking with the Extended Services.

gitgabrio commented 7 months ago

@tiagobento @timwuthenow I think something has to be clarified. The DMN engine has been implemented following the official specifications. I couldnot find any reference to " in the non-strictly-compatible mode of the DMN engine" in the official documentation. My understanding is that it is a feature originally implemented in our DMN engine back in 2016, when DMN 1.1 was still undergoing definition. So, again IINW, using this flag would anyway make the model not-compliant. If the given use-case mentioned by @timwuthenow is not covered by such specifications, the first place to make the request is to the committee itself. One possible workaround could be to use numbers instead of durations (maybe implementing to/from conversion before and after decision evaluation) : hugly as it sounds, there are still lot of time-tracker software that still works that way. The biggest issue I see is that it seems the editor does not validate the model correctly, allowing the insertion of invalid values, and that could be misleading for the end user.

yesamer commented 7 months ago

@gitgabrio @tiagobento @baldimir @timwuthenow As this is a new feature request, and we all agree that change is worthwhile from a DMN user perspective and still no decisions nor commitment has been taken so far, please use this space to start a discussion about the below points:

  1. Defining a detailed requirement for the proposal
  2. Proposing a new RTF based on the above requirement to the OMG group
  3. Evaluate implementing the feature in NOT-STRICT mode before its approval

My opinions:

  1. @timwuthenow request it's reasonable and I believe it will be a great and useful addition for a DMN user. I would evaluate applying that change to all accumulator functions if possible. I guess we need to technically check the feasibility of that.
  2. As the NOT-STRICT mode is a custom extension of the official specs (if I understood correctly), it's fine for me to implement the feature here first, and eventually move it after the RTF is approved. The only constraint I would add is that point 1. must be fully defined and completed and is preferable that we start the process of RTF proposal.

WDYT?

gitgabrio commented 7 months ago

About this My opinion is that, although not part of the specification, the sum function should work with lists of all types that the + operator supports

+ operator support also strings (e.g.): what could be the mean (and other mathematical operations) of them ? What it may be done is to extend the mathematical functions only to data types that can be converted to a meaningfull number (duration/date/date-time).

tiagobento commented 7 months ago

@gitgabrio Yeah.... good point :) I changed the issue title to "durations" instead of "all types that work with the + operator"

tiagobento commented 7 months ago

@gitgabrio The new DMN Editor still can't type-check... so really there's no such issue right now as "editor does not validate the model correctly".

tiagobento commented 7 months ago

From the spec, we have:

The value of a days and time duration can be expressed as a number of seconds. E.g., $value{dtd}$(P1DT1H) = 90000. The $value{dtd}$ function is one-to-one and so it has an inverse function $value{dtd^{-1}}$. E.g., $value{dtd^{-1}}$ (90000) = P1DT1H.

and

The value of a years and months duration can be expressed as a number of months. E.g., $value{ymd}$(P1Y1M) = 13. The $value{ymd}$ function is one-to-one and so it has an inverse function $value{ymd^{-1}}$. E.g., $value{ymd^{-1}}$ (13) = P1Y1M

but such functions $value{dtd}$, $value{ymd}$ and its inverses do not exist, as far as I can tell. If they did, we could do the conversion to seconds/months, execute a sum then convert back to the desired duration format.