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

DROOLS-1701 fix: ranges cannot be (always) promoted to fields #5

Merged

Conversation

evacchi
Copy link

@evacchi evacchi commented Jun 7, 2018

it won't work if boundaries are non-constants!

+17 tests pass, 95 yet to go!

   it won't work if boundaries are non-constants!
@tarilabs
Copy link
Owner

tarilabs commented Jun 7, 2018

If I well understand you are saying, for a range like:
[1..3]
we can have it as a Constant/fields in the generated Java class as it was before.
While for cases like
[nameref1..nameref2]
the above strategy cannot work because nameref1 cannot be resolved statically.

If my understanding is correct: Can you slight refactor this PR so that, if the range endpoint is a NumberLiteral, it take the "old" strategy for Constant/fields, while it goes into this new strategy for any other case, please? In most model I've seen personally [numberliteral..numberliteral] is used a lot. I would like to keep that as a constant if possible. Thanks.
If my understanding is not correct: please explain further? :)

@evacchi
Copy link
Author

evacchi commented Jun 7, 2018

you are right. I have reverted to the (more general) runtime resolution of the boundary expressions. I'll try and handle both the constant and the non-constant case, even if it is slightly non-trivial, because of the way sub-expressions are resolved!

@evacchi
Copy link
Author

evacchi commented Jun 7, 2018

wait, no. that's not what's happening. In fact, the case when it's a numeric is still working as expected

    /**
     * FEEL: [1..2]
     */
    @Override
    public Object apply(EvaluationContext feelExprCtx) {
        return new org.kie.dmn.feel.runtime.impl.RangeImpl(org.kie.dmn.feel.runtime.Range.RangeBoundary.CLOSED, (java.lang.Comparable) K_1, (java.lang.Comparable) K_2, org.kie.dmn.feel.runtime.Range.RangeBoundary.CLOSED);
    }

    public static final java.math.BigDecimal K_1 = new java.math.BigDecimal(1, java.math.MathContext.DECIMAL128);

    public static final java.math.BigDecimal K_2 = new java.math.BigDecimal(2, java.math.MathContext.DECIMAL128);

BTW your observation makes sense and I'll take a look

EDIT: nope, the original version stored the Range as a constant field. Alrighty.

@tarilabs
Copy link
Owner

tarilabs commented Jun 7, 2018

In fact, the case when it's a numeric is still working as expected

not entirely, before that new RangeImpl was an initializer in a field definition (of a constant) in the rendered class.

@evacchi evacchi force-pushed the evacchi-feel-compiler branch from 1055a12 to 4681dbd Compare June 8, 2018 09:09
@evacchi evacchi force-pushed the evacchi-feel-compiler branch from 84682d9 to f612353 Compare June 8, 2018 10:02
@evacchi
Copy link
Author

evacchi commented Jun 8, 2018

down to 88 failed tests, constant field restored for numeric ranges

Copy link
Owner

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request to move a method then we're good to go

@@ -26,6 +33,39 @@
private Comparable lowEndPoint;
private Comparable highEndPoint;

public static RangeImpl of(EvaluationContext ctx, RangeBoundary lowBoundary, Object lowEndPoint, Object highEndPoint, RangeBoundary highBoundary) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this method is not currently shared with the AST-interpreted,
so better to move into CompiledFEELSemanticMappings as sounds analogous to the FEEL list and other constructs there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ay ay captain

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@tarilabs tarilabs merged commit 38c2cd8 into tarilabs:tarilabs-20180808-compilation Jun 8, 2018
@evacchi evacchi deleted the evacchi-feel-compiler branch June 8, 2018 13:29
tarilabs pushed a commit that referenced this pull request Jul 5, 2018
* DROOLS-1701 fix: ranges cannot be (always) promoted to fields

   it won't work if boundaries are non-constants!

* DROOLS-1701 fix: use parenthesized expression in cast (ranges)

* DROOLS-1701 Comparable: do not cast in source code, check in RangeImpl -- for now

* DROOLS-1701 Use ObjectCreation for numeric constants, factory method otherwise

* DROOLS-1701 move range() factory to CompiledFEELSemanticMappings
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.

2 participants