goldmansachs/reladomo

MultiUpdateOperation.combineMultiUpdate incorrectly combines increment on double

neistadt opened this issue · 3 comments

When MultiUpdateOperation tries to combine with another MultiUpdateOperation, the combineOnSameAttribute call incorrectly merges increment on double operations across objects with different primary keys. The setup for this is as follows:

Assume a primary key of Account + Product and a Quantity balance. If you have 6 increment operations as follows:

update1. Account 1, Product 1, increment 100
update2. Account 1, Product 2, increment 100
update3. Account 2, Product 3, increment 100
update4. Account 2, Product 4, increment 100
update5. Account 1, Product 5, increment 100
update6. Account 1, Product 6, increment 100

update1 & 2 will be combined into a MultiUpdateOperation A of:
update Account 1 set quantity = quantity + 100 where product in (1, 2)

update3 & 4 will be combined into a MultiUpdateOperation B of:
update Account 2 set quantity = quantity + 100 where product in (3, 4)

update5 & 6 will be combined into a MultiUpdateOperation C of:
update Account 1 set quantity = quantity + 100 where product in (5, 6)

Next, MultiUpdateOperation A and MultiUpdateOperation C will be combined incorrectly into:
update Account 1 set quantity = quantity + 200 where product in (1, 2, 5, 6)

I believe the combineForSameAttribute funciton in DoubleIncrementUpdateWrapper was only intended to combine increments for objects of the same primary key. But because its combining across different keys, its creating unexpected results.

FYI I have a fix for this issue and will be working on formalizing it

Any update on the fix? I'm planning the next release and it would be good to include this fix.

I should be able to submit a pull request today or tomorrow. Fix is done just waiting for the internal approval.