oracle-samples/clara-rules

NPE from accumulator

victorrodrigueznadq opened this issue · 9 comments

This is the LHS of my rule:

(defrule calculate-dlp-add-ratio
        "Calculate Dlp Add Ratio"
        ;{:salience 1500}
        [VolumeAccumulation (= accumulationLevel VolumeAccumulation$AccumulationLevel/ACCOUNT_SYMBOL)
         (= accumulationCode "nqeAddDlp")
         (= ?exchangeSymbol symbol)
         (= ?acctVolume totalVolume)
         (= ?acctIdentifier accountIdentifier)
         (= ?firmIdentifier firmIdentifier)
         (= ?symbol symbol)
         ( pos? totalVolume)
         ]
        [DlpFirm
         (= ?acctIdentifier mpid)
         (= ?exchangeSymbol symbol)
         (= portId "PrimaryDLP")
         ]

        [?sumTotalVolume <-(acc/sum :totalVolume) :from [VolumeAccumulation
                                                         (= accumulationLevel VolumeAccumulation$AccumulationLevel/EXCHANGE_SYMBOL)
                                                         (= accumulationCode "nqeAddDlp")
                                                         (= symbol ?exchangeSymbol)
                                                     ]]
         [:test (and (some? ?sumTotalVolume) (>= (.doubleValue ?sumTotalVolume) 1)) ]

I'm getting the following NPE from the acc/sum:

java.lang.NullPointerException: Cannot invoke "Object.getClass()" because "x" is null
at clojure.lang.Numbers.ops (Numbers.java:1095)
   clojure.lang.Numbers.add (Numbers.java:155)
   clara.rules.accumulators$sum$fn__7939.invoke (accumulators.cljc:158)
   clojure.lang.ArrayChunk.reduce (ArrayChunk.java:58)
   clojure.core.protocols$fn__8244.invokeStatic (protocols.clj:136)

I've ensured that totalVolume is never null, even by hardcoding the getter to always return 100L. Still get the same error:

I'm stumped. Any suggestions?

Tried acc/min and acc/max and those don't give NPE. acc/sum and acc/average do.

here's the sum function:

(defn sum
  "Returns an accumulator that returns the sum of values of a given field"
  [field]
  (accum
   {:initial-value 0
    :reduce-fn (fn [total item]
                 (+ total (field item)))
    :retract-fn (fn [total item]
                  (- total (field item)))
    :combine-fn +}))

Maybe the total is null? how could this be?

Maybe the total is null? how could this be?

I don't believe that to be a possibility, as we should always be starting at zero and applying increments and decrements based on the direction(insert/retract) of the facts.

Tried acc/min and acc/max and those don't give NPE. acc/sum and acc/average do.

That seems to be somewhat odd, min/max handle the initial value being nil but any subsequent comparisons i would have assumed to fail if they encounter a nil.

For the VolumeAccumulation fact, is it a POJO or a Clojure record?

Maybe the total is null? how could this be?

I don't believe that to be a possibility, as we should always be starting at zero and applying increments and decrements based on the direction(insert/retract) of the facts.

Tried acc/min and acc/max and those don't give NPE. acc/sum and acc/average do.

That seems to be somewhat odd, min/max handle the initial value being nil but any subsequent comparisons i would have assumed to fail if they encounter a nil.

For the VolumeAccumulation fact, is it a POJO or a Clojure record?

@EthanEChristian it's a POJO.

@victorrodrigueznadq,
That is probably the issue here, while the syntactic sugar exists in the LHS for bindings and variable access within constraints for POJOs, ie.

(= ?exchangeSymbol symbol)
(= ?acctVolume totalVolume)
(= ?acctIdentifier accountIdentifier)
(= ?firmIdentifier firmIdentifier)

The same doesn't apply to accumulators that have field accessors, so i believe that the application of the keyword accessor(:totalVolume) is likely the root of the issue here. In the sense that a Keyword applied to anything that isn't a map or record is likely to return nil.

While not as pretty, i believe that the NPE can be avoided by creating an accessor on the fly for the POJO in question, something like:

[?sumTotalVolume <-(acc/sum #(.getTotalVolume %)) :from [VolumeAccumulation
                                                         (= accumulationLevel VolumeAccumulation$AccumulationLevel/EXCHANGE_SYMBOL)
                                                         (= accumulationCode "nqeAddDlp")
                                                         (= symbol ?exchangeSymbol)
                                                     ]]

As to:

Tried acc/min and acc/max and those don't give NPE. acc/sum and acc/average do.

The only thing i can conclude would possibly be that there only ever exists one matching VolumeAccumulation, such that the max/min accumulator would never execute twice, exposing the NPE.

@EthanEChristian that worked. thanks! is there a plan to add the missing "sugar?"

@victorrodrigueznadq I will certainly log an issue to investigate the possibility of adding that support, as it feels like a pretty easy gotcha to bump into.

That being said, i'm not sure of how i'd tackle it in the current implementation but it certainly feels like something that should be mulled over.

I will post back with the issue number in a bit.

@victorrodrigueznadq
I have created #499 as an enhancement to hopefully rectify the discrepancy with the application of syntactic sugar.

@victorrodrigueznadq I have created #499 as an enhancement to hopefully rectify the discrepancy with the application of syntactic sugar.

thanks @EthanEChristian good luck!