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

Two different implementations of FEEL execution #73

Closed
Tracked by #997 ...
baldimir opened this issue Dec 16, 2022 · 1 comment · Fixed by apache/incubator-kie-drools#5901
Closed
Tracked by #997 ...

Two different implementations of FEEL execution #73

baldimir opened this issue Dec 16, 2022 · 1 comment · Fixed by apache/incubator-kie-drools#5901
Assignees
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:enhancement Something that already exists needs to be improved type:tech-debt Things that were left behind an may harm us in the future.

Comments

@baldimir
Copy link

baldimir commented Dec 16, 2022

Currently there are two different implementations that execute FEEL - one in interpreted mode and one in compiled Java mode (code generated). The execution uses some common mode on some places, but not everywhere. This should be fixed. Business logic of the evaluation should be encapsulated in a common module which is used by both interpreted execution and compiled execution.

Pointers:

@baldimir baldimir converted this from a draft issue Dec 16, 2022
@baldimir baldimir added type:enhancement Something that already exists needs to be improved area:dmn Related to DMN decisions labels Dec 16, 2022
@gitgabrio gitgabrio self-assigned this Jan 16, 2023
@yesamer yesamer added the area:engine Related to the runtime engines label Jan 24, 2024
@yesamer yesamer moved this from 📋 Backlog to 🅿️ Parking Lot in 🦉 KIE Podling Board Jan 24, 2024
@yesamer yesamer moved this from 🅿️ Parking Lot to 📋 Backlog in 🦉 KIE Podling Board Jan 24, 2024
@gitgabrio gitgabrio added the type:tech-debt Things that were left behind an may harm us in the future. label May 6, 2024
@gitgabrio
Copy link

gitgabrio commented May 7, 2024

Based on analysys, I found that FEEL usage itself is not duplicated, i.e. both code paths ultimately invoke
FEELFunction#invokeReflectively(EvaluationContext, Object[])}

The difference is the overall "context" and the way messages/errors are dealt with:

  1. in the intepreted way, the MsgUtil.report is used in case of errors, and the returned EvaluatorResultImpl has a flag to indicate a failure
  2. in the compiled way, the EvaluationContext.notifyEvent() is invoked in case of errors, and the returned object is the result of function evaluation itself.

Merging the two paths seems pretty error prone and anyway the core FEEL implementation itself seems not duplicated.

@baldimir @yesamer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dmn Related to DMN area:engine Related to the runtime engines type:enhancement Something that already exists needs to be improved type:tech-debt Things that were left behind an may harm us in the future.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants