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.

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

Fix 4580 Add PostgreSql Select Distinct On #4584

Merged
merged 4 commits into from
Sep 19, 2023

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Aug 31, 2023

Initial grammar and fixture test
Mixin for Distinct On columns
Annotate Distinct On using the basic rule "The DISTINCT ON column(s) must match the leftmost ORDER BY column(s)" see https://github.com/cockroachdb/cockroach/blob/b994d025c678f495cb8b93044e35a8c59595bd78/pkg/sql/opt/optbuilder/distinct.go#L91. This should catch at compile time, instead of runtime where columns are not matching.

fixes #4580

@griffio griffio changed the title Add PostgreSql Distinct On Fix 4580 Add PostgreSql Select Distinct On Aug 31, 2023
@griffio griffio marked this pull request as ready for review September 4, 2023 13:08
Initial grammar and fixture

TODO
Validate Distinct On Columns match  Order By Columns
Integration Test
Initial attempt at annotating errors for DISTINCT ON

"DISTINCT ON expression(s) must match the leftmost ORDER BY expression(s)"
I looked at the CockRoachDB implementation (compatible with Postgres)

Essentially, the simplest check is to ensure the ORDER BY columns exist in the DISTINCT ON columns.
Remove the negation
@AlecKazakova AlecKazakova merged commit 930be92 into cashapp:master Sep 19, 2023
7 checks passed
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
* Add PostgreSql Distinct On

Initial grammar and fixture

TODO
Validate Distinct On Columns match  Order By Columns
Integration Test

* Add DistinctOnExpressionMixin

Initial attempt at annotating errors for DISTINCT ON

"DISTINCT ON expression(s) must match the leftmost ORDER BY expression(s)"

* Add mixin to handle annotated errors

I looked at the CockRoachDB implementation (compatible with Postgres)

Essentially, the simplest check is to ensure the ORDER BY columns exist in the DISTINCT ON columns.

* Can use none instead of any

Remove the negation
@lnhrdt
Copy link
Contributor

lnhrdt commented Apr 19, 2024

I'm finally getting around to upgrading my project to 2.0.2 to take advantage of these changes. Did this change also have the effect of removing support for just SELECT DISTINCT col1, col2 and requiring the SELECT DISTINCT ON (col1, col2) col1, col2 syntax?

I'm finding that to be the case but it's a backwards incompatible change with my migrations. Having to change migrations changes their hash, which is a tricky proposition using a migration tool like Flyway.

@griffio
Copy link
Contributor Author

griffio commented Apr 19, 2024

I'm finally getting around to upgrading my project to 2.0.2 to take advantage of these changes. Did this change also have the effect of removing support for just SELECT DISTINCT col1, col2 and requiring the SELECT DISTINCT ON (col1, col2) col1, col2 syntax?

I'm finding that to be the case but it's a backwards incompatible change with my migrations. Having to change migrations changes their hash, which is a tricky proposition using a migration tool like Flyway.

@lnhrdt
Raise a new issue - I will fix it in a PR as it looks like the grammar is not matching for SELECT DISTINCT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for PostgreSQL DISTINCT ON syntax
3 participants