feat: add support for interval aggregation

graphile / pg-aggregates

Aggregates for PostGraphile connections

83 stars 17 forks source link

feat: add support for interval aggregation #58

Closed Codykilpatrick closed 9 months ago

Codykilpatrick commented 1 year ago

Description

Expands the AggregateSpecsPlugin to check for Interval types as well as numeric.

Possible breaking change: I removed the isNonNull requirement and the coalesce from Sum. Now people who were properly getting their aggregates coalsced into 0 will now receive null.

Performance impact

Unknown

Security impact

Unknown

Checklist

[x] My code matches the project's code style and yarn lint:fix passes.
[x] I've added tests for the new feature, and yarn test passes.
[x] I have detailed the new feature in the relevant documentation.
[x] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
[x] If this is a breaking change I've explained why.

This will be my first contribution so please let me know if I missed anything, it seemed relatively simple though!

Codykilpatrick commented 1 year ago

Marked this as a draft for now, after thinking about it isIntervalLike does need to be separate from isNumberLike since interval types don't support aggregations like VariancePop.

Codykilpatrick commented 1 year ago

Currently this works for all aggregates except SUM and that is due to the coalesce here: https://github.com/graphile/pg-aggregates/blob/975eefb2c13589fe856d14ba66830e29d0b48d72/src/AggregateSpecsPlugin.ts#L76

When attempting an aggregate sum query on an interval type it generates the following error:

"errors": [
    {
      "message": "COALESCE types interval and integer cannot be matched",

Is it possible to remove the coalesce and the "isNonNull" property? I did some local testing and no issues with aggregating both null and non-null interval/numeric data types but I might be missing some edge cases.

I think though that would be a breaking change since people who are expecting 0 in that situations would now be getting null but am curious to your thoughts.

Codykilpatrick commented 1 year ago

@benjie Are their certain tests you would like to see added for this PR?

benjie commented 1 year ago

It'd be nice if we actually had any tests in this repo :grimacing:

I'm working on Worker for the next week or so then I'll be back to thinking about PostGraphile again. Sorry for the delay, but I've neglected Worker for too long.

Codykilpatrick commented 1 year ago

All good! Thank you for the response!

Codykilpatrick commented 10 months ago

@benjie Curious if there has been any movement on this one!

benjie commented 9 months ago

Sorry, this is still on my TODO list, but I'm currently focussed on earning some money; I spent altogether too much time on OSS last year and not enough on paying the bills.

Codykilpatrick commented 9 months ago

All good! bills are definitely the priority here 😆

benjie commented 9 months ago

Released in @graphile/pg-aggregates@0.2.0-beta.5