tomjaguarpaw / haskell-opaleye

Other
602 stars 115 forks source link

disallow `orderAggregate` with `distinctAggregator` #587

Closed stevemao closed 8 months ago

stevemao commented 8 months ago

Using orderAggregate with distinctAggregator will generate something like this

ARRAY_AGG(DISTINCT "inner2_5" ORDER BY "a" ASC NULLS LAST) as "result2_5"

Which will cause

SqlError {sqlState = \"42P10\", sqlExecStatus = FatalError, sqlErrorMsg = \"in an aggregate with DISTINCT, ORDER BY expressions must appear in argument list\", sqlErrorDetail = \"\", sqlErrorHint = \"\"}

The correct way to do it is to use distinctOn in the inner sql

However, based on the current type system is might be hard to create a type error. It might be possible to create a warning?

stevemao commented 8 months ago

Or perhaps we can deprecate distinctAggregator in favour of distinctOn? I can't think any case distinctAggregator can do but distinctOn can't

tomjaguarpaw commented 8 months ago

Can you please share a small example that generates this crashing code?

stevemao commented 8 months ago

Yes, just clone https://github.com/stevemao/opaleye-order-distinct-aggregator and run docker compose up.

tomjaguarpaw commented 8 months ago

Thanks. I believe this will be fixed by https://github.com/tomjaguarpaw/haskell-opaleye/pull/586. Perhaps you'd like to try it.

stevemao commented 8 months ago

@tomjaguarpaw still the same error https://github.com/stevemao/opaleye-order-distinct-aggregator/tree/aggregate

tomjaguarpaw commented 8 months ago

Could you double check? This works fine, for example: https://github.com/tomjaguarpaw/haskell-opaleye/commit/a9e6fe209a088d90996d4a2cbf03dd4b42036b1a

(I don't have the ability to run Docker here)

tomjaguarpaw commented 8 months ago

(And see here for the test run: https://github.com/tomjaguarpaw/haskell-opaleye/actions/runs/7669040691)

tomjaguarpaw commented 8 months ago

If you still think there's a problem, could you come up with a failing test case that I can add to Test.hs?

stevemao commented 8 months ago

My bad. it is indeed working now https://github.com/stevemao/opaleye-order-distinct-aggregator/actions/runs/7676483130/job/20924091643

tomjaguarpaw commented 8 months ago

Great, glad it works for you.