search

camunda / feel-scala

FEEL parser and interpreter written in Scala
https://camunda.github.io/feel-scala/
Apache License 2.0
124 stars 51 forks source link

Add new built-in range functions #178

Closed felideon closed 2 years ago

felideon commented 4 years ago

The built-in range functions below were added to the DMN 1.3 standard (see § 10.3.4.7 Range Functions). These functions would be helpful when writing business rules with FEEL:

The following set of functions establish relationships between single scalar values and ranges of such values. All functions in this list take two arguments and return True if the relationship between the argument holds, or False otherwise.

image

image

image

image

image

image

image

image

image

saig0 commented 4 years ago

@felideon thank you for raising this up! I collected the relevant content from the specification here.

In which context do you need these functions? Camunda BPM platform 7 or Zeebe?

Currently, we don't focus on DMN 1.3. It may take a while before someone from Camunda/Zeebe will implement this. So, feel free to contribute by creating pull requests!

felideon commented 4 years ago

Hi @saig0, thanks. This would be for the Camunda BPMN/DMN engine. The company I work for is an Enterprise customer.

Though tempting, I'll have to consider the feasibility of working on a PR. I've never worked with Scala (let alone Java).

kaaquist commented 3 years ago

@saig0, is this here up for grabs? Seems like an old issue 😄 But I guess I could create the RangeBuildinFunctions as mentioned above.

saig0 commented 3 years ago

@kaaquist go for it :rocket:

Since this is a big issue with many functions, I would prefer to create one PR for each function. So, I can review the PRs faster and give feedback.

kaaquist commented 3 years ago

@saig0 I thought that I would add a ValRange case class in the Val.scala. I can see from the above that we will be adding the ability to do inclusive and exclusive ranges. I'm not aware of a Java or Scala function that can do that. Maybe you know? I have started out creating a ValRange which take a string. See WIP PR -> #345

kaaquist commented 3 years ago

I have tried not to give ValString as input, and can see that you have created an Interval in the FeelInterpreter.scala which is tried to be used, but it end up with an error. Test case:

it should "return true because range end is included and point is higher" in {

    eval(" before([1..10], 20)") should be(ValBoolean(true))
  }

I can see that we get a PositionalFunctionParameters(List(Interval(ClosedIntervalBoundary(ConstNumber(1)),ClosedIntervalBoundary(ConstNumber(10))), ConstNumber(20))) And then the next step is the FeelInterpreter, but something does not go as expected and I get an error: 

23:21:37.240 [ScalaTest-run-running-BuiltinRangeFunctionTest] WARN  org.camunda.feel.FeelEngine - Failed to invoke function: no variable found for name 'cellInput'

ValNull was not equal to ValBoolean(true)
ScalaTestFailureLocation: org.camunda.feel.impl.builtin.BuiltinRangeFunctionTest at (BuiltinRangeFunctionTest.scala:74)
Expected :ValBoolean(true)
Actual   :ValNull
<Click to see difference>

org.scalatest.exceptions.TestFailedException: ValNull was not equal to ValBoolean(true)
    at org.scalatest.MatchersHelper$.indicateFailure(MatchersHelper.scala:343)
    at org.scalatest.Matchers$ShouldMethodHelper$.shouldMatcher(Matchers.scala:6723)
    at org.scalatest.Matchers$AnyShouldWrapper.should(Matchers.scala:6759)
    at org.camunda.feel.impl.builtin.BuiltinRangeFunctionTest.$anonfun$new$13(BuiltinRangeFunctionTest.scala:74)
    at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
    at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
    at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
    at org.scalatest.Transformer.apply(Transformer.scala:22)
    at org.scalatest.Transformer.apply(Transformer.scala:20)
    at org.scalatest.FlatSpecLike$$anon$5.apply(FlatSpecLike.scala:1682)
    at org.scalatest.TestSuite.withFixture(TestSuite.scala:196)
    at org.scalatest.TestSuite.withFixture$(TestSuite.scala:195)
    at org.scalatest.FlatSpec.withFixture(FlatSpec.scala:1685)
    at org.scalatest.FlatSpecLike.invokeWithFixture$1(FlatSpecLike.scala:1680)
    at org.scalatest.FlatSpecLike.$anonfun$runTest$1(FlatSpecLike.scala:1692)
    at org.scalatest.SuperEngine.runTestImpl(Engine.scala:286)
    at org.scalatest.FlatSpecLike.runTest(FlatSpecLike.scala:1692)
    at org.scalatest.FlatSpecLike.runTest$(FlatSpecLike.scala:1674)
    at org.scalatest.FlatSpec.runTest(FlatSpec.scala:1685)
    at org.scalatest.FlatSpecLike.$anonfun$runTests$1(FlatSpecLike.scala:1750)
    at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:393)
    at scala.collection.immutable.List.foreach(List.scala:333)
    at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:381)
    at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:370)
    at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:407)
    at scala.collection.immutable.List.foreach(List.scala:333)
    at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:381)
    at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:376)
    at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:458)
    at org.scalatest.FlatSpecLike.runTests(FlatSpecLike.scala:1750)
    at org.scalatest.FlatSpecLike.runTests$(FlatSpecLike.scala:1749)
    at org.scalatest.FlatSpec.runTests(FlatSpec.scala:1685)
    at org.scalatest.Suite.run(Suite.scala:1124)
    at org.scalatest.Suite.run$(Suite.scala:1106)
    at org.scalatest.FlatSpec.org$scalatest$FlatSpecLike$$super$run(FlatSpec.scala:1685)
    at org.scalatest.FlatSpecLike.$anonfun$run$1(FlatSpecLike.scala:1795)
    at org.scalatest.SuperEngine.runImpl(Engine.scala:518)
    at org.scalatest.FlatSpecLike.run(FlatSpecLike.scala:1795)
    at org.scalatest.FlatSpecLike.run$(FlatSpecLike.scala:1793)
    at org.scalatest.FlatSpec.run(FlatSpec.scala:1685)
    at org.scalatest.tools.SuiteRunner.run(SuiteRunner.scala:45)
    at org.scalatest.tools.Runner$.$anonfun$doRunRunRunDaDoRunRun$13(Runner.scala:1349)
    at org.scalatest.tools.Runner$.$anonfun$doRunRunRunDaDoRunRun$13$adapted(Runner.scala:1343)
    at scala.collection.immutable.List.foreach(List.scala:333)
    at org.scalatest.tools.Runner$.doRunRunRunDaDoRunRun(Runner.scala:1343)
    at org.scalatest.tools.Runner$.$anonfun$runOptionallyWithPassFailReporter$24(Runner.scala:1033)
    at org.scalatest.tools.Runner$.$anonfun$runOptionallyWithPassFailReporter$24$adapted(Runner.scala:1011)
    at org.scalatest.tools.Runner$.withClassLoaderAndDispatchReporter(Runner.scala:1509)
    at org.scalatest.tools.Runner$.runOptionallyWithPassFailReporter(Runner.scala:1011)
    at org.scalatest.tools.Runner$.run(Runner.scala:850)
    at org.scalatest.tools.Runner.run(Runner.scala)
    at org.jetbrains.plugins.scala.testingSupport.scalaTest.ScalaTestRunner.runScalaTest2or3(ScalaTestRunner.java:38)
    at org.jetbrains.plugins.scala.testingSupport.scalaTest.ScalaTestRunner.main(ScalaTestRunner.java:25)

So it seems that there is a default value in play: no variable found for name 'cellInput' .. But I'm not totally sure. Maybe you can point me in some direction?

saig0 commented 3 years ago

@kaaquist this is the right direction. In the parser, we could reuse (parts of) the Interval. But we need to adjust the interpreter. At the moment, the Interval is used to compare the input value with the given interval. Here, the input value is read from the context (using the default name cellInput).

We could create a new data type ConstRange similar to Interval. In the interpreter, we could distinguish both types and use the range for the new functions.

BTW: the new data type for range is also part of #293.

kaaquist commented 3 years ago

@saig0 I have updated the first PR #345, please have a look.

kaaquist commented 3 years ago

@saig0 seems like something is missing in the overlaps function. I can't get it to pass when doing the following tests: overlaps ([1..5], (5..8]) = false return true instead of false.
overlaps ([1..5), [5..8]) = false return true instead of false. overlaps ([1..5), (5..8]) = false return true instead of false. Missing meaning some check in the description above. All other tests passes.

Please see code here #353

saig0 commented 3 years ago

seems like something is missing in the overlaps function

True :+1: There is a bug in the spec description. In the first part of the condition, it should be range2.start included instead of range2.end included since it compares the start of range2.