Open ThatJoeMoore opened 1 month ago
Hello @ThatJoeMoore , thanks a lot for the detailed information!
I do agree with the mistrust of instanceof, and would prefer using the type of the column or as a last solution a structural check.
To get started, would you be able to add a test that reproduce this use-case? That would make our discussion easier, without having to do all the work at once.
Problem
The
aggregateSum
function doesn't work for values in aDecimal
column.Given a table like
Running
Results in
Instead of
Looking deeper, it looks like similar problems exist for
avg
too, and possibly formin
andmax
(haven't tested those yet).I can get the math to work properly if I pass a primitive number to Prismock when I insert data, but then the results of queries don't match the types that come out of Prisma.
Proposed Solution
If one or both of the inputs is a Decimal type, use
Decimal.add
to add them together. Something like:This wraps the values in a
new Decimal
to make sure both sides are the correct type before adding.This obviously requires updating the types around it to use number | Decimal.
The use of
instanceof
is illustrative; I'd consider having a more structural type check instead (specifically, you can check if it has fields ofd: number[]
,e: number
, ands: number
. But that's probably just my inherent mistrust ofinstanceof
in JS talking :) .If we wanted to get more advanced, we could look at the types of the column in the model and see if it's a decimal type, in which case, we switch to a decimal-friendly version of the aggregator. The same approach could be extended for any other type-specific aggregation variants.
I can get a PR in to do the work, I'd just love guidance on how to implement the fix. The two big questions:
instanceof
(simple but can be broken by arcane dependency resolution issues) or a structural type check (less clear and is, at best, a really good guess at the type, but resilient against weird dependency issues)?Personally, I think I'm leaning towards detecting if we have a 'special' column type and swapping out the implementation of the aggregators accordingly. Probably just for
number
andDecimal
for now, but it could be expanded if Prisma adds aggregation support for other types too.