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

NPE from accumulator #498

Open
victorrodrigueznadq opened this issue May 30, 2024 · 9 comments
Open

NPE from accumulator #498

victorrodrigueznadq opened this issue May 30, 2024 · 9 comments
Assignees
Labels

Comments

@victorrodrigueznadq
Copy link

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?

@victorrodrigueznadq
Copy link
Author

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

@victorrodrigueznadq
Copy link
Author

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?

@EthanEChristian
Copy link
Contributor

EthanEChristian commented May 31, 2024

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?

@victorrodrigueznadq
Copy link
Author

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.

@EthanEChristian
Copy link
Contributor

@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.

@victorrodrigueznadq
Copy link
Author

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

@EthanEChristian
Copy link
Contributor

@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.

@EthanEChristian
Copy link
Contributor

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

@victorrodrigueznadq
Copy link
Author

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

thanks @EthanEChristian good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants