[DROOLS-1175] infer numeric type for sum expression in an accumulate …

[mariofusco]

…pattern

[ge0ffrey]

For clarity, I believe they should have been named "sumInteger" etc, but I believe that ship has already sailed, didn't it?

[ge0ffrey]

If you've seen any previous mails with stacktraces: those don't count - I had another local branch vs your branch name conflict issue locally. I deleted all those from the Github page, so only relevant comments are still on this github PR page.

There's still an issue: with a sum of integers, the call to IntegerSumAccumulateFunction.accumulate(Serializable context, Object value) actually delivers a Long value (which is nicely cast to an int, except in my drools fork (which fixes some bugs) where it triggers a ClassCastException).

It shouldn't be deliver an Long value. For example, in this rule:

rule "requiredCpuPowerTotal"
    when
        $computer : CloudComputer($cpuPower : cpuPower)
        $requiredCpuPowerTotal : Integer(this > $cpuPower) from accumulate(
            CloudProcess(
                computer == $computer,
                $requiredCpuPower : requiredCpuPower),
            sum($requiredCpuPower)
        )
    then
        scoreHolder.addHardConstraintMatch(kcontext, $cpuPower - $requiredCpuPowerTotal);
end

with this class

public class CloudProcess {
    private int requiredCpuPower;

    public int getRequiredCpuPower() {
        return requiredCpuPower;
    }
} 

That sum value should definitely be an int or Integer.

[ge0ffrey]

I've send in a pull request for your branch that fixes the BigDecimal rounding issue and exposes the Long casting issue: mariofusco#1

[mariofusco]

@ge0ffrey I merged your PR into mine but I still have a doubt: can we safely assume that an accumulator of Numbers of a given class will be exclusively fed by instances of that class? E.g. is it correct that an accumulator on BigDecimal eventually throws a CCE if for some reason is passed with a double? I must admit that at the moment I have no evidence of how this could happen, I just wanted to be sure that this is acceptable. Also what happens if it is passed with a null? I'm afraid this is far more probable and at the moment it will throw a NPE. What should we do in that case? Propagate the NPE? Considering it as a zero?

That said if I have understood correctly you're saying that this current implementation triggers an error in that requiredCpuPowerTotal rule? If so how could I reproduce this issue?

[ge0ffrey]

This PR fails, probably because of the ClassCastExceptions that are happening. Before we revert those casting and head back to doubleValue() (which is a rounding error when done some BigDecimals and even some longs), we might want to check if we can fix that and keep the casts.

About a double being passed to a BigDecimal accumulate: isn't that impossible because the compile time model guarantees that the parameter is of type BigDecimal? It's like saying that the method AtomicLong.addAndGet(long delta) should internally deal with delta not being a long but a double, no?
Yes, our unit tests now break with ClassCastException, but isn't that normal as they're trying to put ints in a Double accumulation etc? Maybe for backwards compatility we should allow the double accumulation to avoid the cast if it's not instanceof and fallback to doubleValue().

About nulls: if for backwards compatibility you want to keep null + null = null and null + 7 = 7 etc, I wouldn't mind, but otherwise I 'd just fail fast on that too.
Fail fast isn't bad: it's just saying we're not allowing you to do that now because we believe you'll only use it unintended in error - and once someone can deliver a use case that does really need it, we 'll put it in there in the next version. Josh Bloch's advice for any feature: When in doubt, leave it out. (especially edge cases are good fail fast candidates until proven useful).

Let's discuss Wednesday afternoon or Friday when I back in the home-office?

[mariofusco]

Ok, let's discuss this when you'll be back at home. For now I just want to give a look at that CCE you mentioned (and possibly fix it if I'll have time). How can I reproduce it?

[ge0ffrey]

To reproduce run AccumulateTest or any of the others that are failing on Jenkins.

[mariofusco]

Replaced by #867