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.

Sign up for GitHub

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

Jump to bottom

disallow orderAggregate with distinctAggregator #587

Closed
stevemao opened this issue Jan 18, 2024 · 10 comments
Closed

disallow orderAggregate with distinctAggregator #587

stevemao opened this issue Jan 18, 2024 · 10 comments

Comments

@stevemao
Copy link
Contributor

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
Copy link
Contributor Author

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

@tomjaguarpaw
Copy link
Owner

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

@stevemao
Copy link
Contributor Author

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

tomjaguarpaw added a commit that referenced this issue Jan 21, 2024
@tomjaguarpaw
Avoid rebinding PrimExprs that were created in the Aggregator itself
94f887c 
It's redundant because we only need to rebind PrimExprs that came from
a lateral subquery.  As explained at [1], rather than carefully
analysing which PrimExprs came from a lateral subquery we can just
rebind everything.  A previous commit [2] changed things so that
everything mentioned in aggregator order expressions is rebound.
Instead we probably should have done what this commit does, that is,
added an Unpackspec constraint to aggregate.

Fixes #587

This is a simpler approach to resolving the issues discussed at

* #585
* #578

This still suffers from the problem described at:

#578 (comment)

i.e. if we find a way of duplicating field names, such as

    O.asc (\x -> snd x O..++ snd x)

then we can still create crashing queries.  The benefit of this
comment is that there is a way of generating non-crashing queries!

[1] https://github.com/tomjaguarpaw/haskell-opaleye/blob/52a4063188dd617ff91050dc0f2e27fc0570633c/src/Opaleye/Internal/Aggregate.hs#L111-L114

[2] d848317, part of
    #576
@tomjaguarpaw
Copy link
Owner

Thanks. I believe this will be fixed by #586. Perhaps you'd like to try it.

stevemao added a commit to stevemao/opaleye-order-distinct-aggregator that referenced this issue Jan 26, 2024
@stevemao
try tomjaguarpaw/haskell-opaleye#587 (comment)
3bd46a7
@stevemao
Copy link
Contributor Author

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

tomjaguarpaw added a commit that referenced this issue Jan 26, 2024
@tomjaguarpaw
Avoid rebinding PrimExprs that were created in the Aggregator itself
0a2ef1b 
It's redundant because we only need to rebind PrimExprs that came from
a lateral subquery.  As explained at [1], rather than carefully
analysing which PrimExprs came from a lateral subquery we can just
rebind everything.  A previous commit [2] changed things so that
everything mentioned in aggregator order expressions is rebound.
Instead we probably should have done what this commit does, that is,
added an Unpackspec constraint to aggregate.

Fixes #587

This is a simpler approach to resolving the issues discussed at

* #585
* #578

This still suffers from the problem described at:

#578 (comment)

i.e. if we find a way of duplicating field names, such as

    O.asc (\x -> snd x O..++ snd x)

then we can still create crashing queries.  The benefit of this
comment is that there is a way of generating non-crashing queries!

[1] https://github.com/tomjaguarpaw/haskell-opaleye/blob/52a4063188dd617ff91050dc0f2e27fc0570633c/src/Opaleye/Internal/Aggregate.hs#L111-L114

[2] d848317, part of
    #576
tomjaguarpaw added a commit that referenced this issue Jan 26, 2024
@tomjaguarpaw
587 example
a9e6fe2 
#587
@tomjaguarpaw
Copy link
Owner

Could you double check? This works fine, for example: a9e6fe2

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

@tomjaguarpaw
Copy link
Owner

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

@tomjaguarpaw
Copy link
Owner

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
Copy link
Contributor Author

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

@tomjaguarpaw
Copy link
Owner

Great, glad it works for you.

tomjaguarpaw added a commit that referenced this issue Feb 6, 2024
@tomjaguarpaw
Avoid rebinding PrimExprs that were created in the Aggregator itself
3ae95d4 
It's redundant because we only need to rebind PrimExprs that came from
a lateral subquery.  As explained at [1], rather than carefully
analysing which PrimExprs came from a lateral subquery we can just
rebind everything.  A previous commit [2] changed things so that
everything mentioned in aggregator order expressions is rebound.
Instead we probably should have done what this commit does, that is,
added an Unpackspec constraint to aggregate.

Fixes #587

This is a simpler approach to resolving the issues discussed at

* #585
* #578

This still suffers from the problem described at:

#578 (comment)

i.e. if we find a way of duplicating field names, such as

    O.asc (\x -> snd x O..++ snd x)

then we can still create crashing queries.  The benefit of this
comment is that there is a way of generating non-crashing queries!

[1] https://github.com/tomjaguarpaw/haskell-opaleye/blob/52a4063188dd617ff91050dc0f2e27fc0570633c/src/Opaleye/Internal/Aggregate.hs#L111-L114

[2] d848317, part of
    #576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tomjaguarpaw @stevemao