Skip to content

Commit

Permalink
Fix 4580 Add PostgreSql Select Distinct On (#4584)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
griffio authored and hfhbd committed Apr 2, 2024
1 parent 915f355 commit 8597faf
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
parserImports=[
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ADD"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ABORT"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ALL"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ALTER"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ALWAYS"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.AS"
Expand All @@ -27,6 +28,7 @@
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DEFAULT"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DELETE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DESC"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DISTINCT"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DO"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DOT"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.DROP"
Expand All @@ -40,6 +42,7 @@
"static com.alecstrong.sql.psi.core.psi.SqlTypes.FROM"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.GENERATED"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.GROUP"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.HAVING"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ID"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.IF"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.IGNORE"
Expand All @@ -63,13 +66,15 @@
"static com.alecstrong.sql.psi.core.psi.SqlTypes.REPLACE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.ROLLBACK"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.RP"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.SELECT"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.SET"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.STRING"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.TO"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.TRUE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.UNIQUE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.UPDATE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.USING"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.VALUES"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.WHERE"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.WITH"
"static com.alecstrong.sql.psi.core.psi.SqlTypes.WITHOUT"
Expand All @@ -91,6 +96,7 @@ overrides ::= type_name
| extension_expr
| extension_stmt
| create_index_stmt
| select_stmt

column_constraint ::= [ CONSTRAINT {identifier} ] (
PRIMARY KEY [ ASC | DESC ] {conflict_clause} |
Expand Down Expand Up @@ -319,6 +325,18 @@ alter_table_alter_column ::= ALTER [COLUMN] {column_name}
pin = 1
}

distinct_on_expr ::= DISTINCT ON LP {result_column} ( COMMA {result_column} ) * RP {
mixin = "app.cash.sqldelight.dialects.postgresql.grammar.mixins.DistinctOnExpressionMixin"
implements = "com.alecstrong.sql.psi.core.psi.SqlCompositeElement"
}

select_stmt ::= SELECT ( [ distinct_on_expr ] | [ DISTINCT | ALL ] ) {result_column} ( COMMA {result_column} ) * [ FROM {join_clause} ] [ WHERE <<expr '-1'>> ] [{group_by}] [HAVING <<expr '-1'>>] | VALUES {values_expression} ( COMMA {values_expression} ) * {
extends = "com.alecstrong.sql.psi.core.psi.impl.SqlSelectStmtImpl"
implements = "com.alecstrong.sql.psi.core.psi.SqlSelectStmt"
override = true
pin = 1
}

compound_select_stmt ::= [ {with_clause} ] {select_stmt} ( {compound_operator} {select_stmt} ) * [ ORDER BY {ordering_term} ( COMMA {ordering_term} ) * ] [ LIMIT {limiting_term} ] [ ( OFFSET | COMMA ) {limiting_term} ] [ FOR UPDATE [ 'SKIP' 'LOCKED' ] ] {
extends = "com.alecstrong.sql.psi.core.psi.impl.SqlCompoundSelectStmtImpl"
implements = "com.alecstrong.sql.psi.core.psi.SqlCompoundSelectStmt"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package app.cash.sqldelight.dialects.postgresql.grammar.mixins

import app.cash.sqldelight.dialects.postgresql.grammar.psi.PostgreSqlDistinctOnExpr
import com.alecstrong.sql.psi.core.SqlAnnotationHolder
import com.alecstrong.sql.psi.core.psi.QueryElement
import com.alecstrong.sql.psi.core.psi.SqlColumnName
import com.alecstrong.sql.psi.core.psi.SqlCompositeElementImpl
import com.alecstrong.sql.psi.core.psi.SqlResultColumn
import com.alecstrong.sql.psi.core.psi.SqlSelectStmt
import com.alecstrong.sql.psi.core.psi.impl.SqlCompoundSelectStmtImpl
import com.intellij.lang.ASTNode
import com.intellij.psi.PsiElement
import com.intellij.psi.util.PsiTreeUtil

internal abstract class DistinctOnExpressionMixin(node: ASTNode) :
SqlCompositeElementImpl(node), PostgreSqlDistinctOnExpr {

private val distinctOnColumns get() = children.filterIsInstance<SqlResultColumn>()

override fun queryAvailable(child: PsiElement): Collection<QueryElement.QueryResult> {
return (parent as SqlSelectStmt).queryExposed()
}

// Some idea of the basic validation finds the ORDER BY columns in the DISTINCT ON
// https://github.com/cockroachdb/cockroach/blob/b994d025c678f495cb8b93044e35a8c59595bd78/pkg/sql/opt/optbuilder/distinct.go#L87
override fun annotate(annotationHolder: SqlAnnotationHolder) {
super.annotate(annotationHolder)

val orderByTerms = (parent.parent as SqlCompoundSelectStmtImpl).orderingTermList

val orderByColumnNames =
orderByTerms.mapNotNull { PsiTreeUtil.findChildOfType(it, SqlColumnName::class.java) }

val distinctOnColumnNames =
distinctOnColumns.mapNotNull { PsiTreeUtil.findChildOfType(it, SqlColumnName::class.java) }

orderByColumnNames.zip(distinctOnColumnNames) { orderByCol, _ ->
if (distinctOnColumnNames.none { distinctOnCol -> distinctOnCol.textMatches(orderByCol) }) {
annotationHolder.createErrorAnnotation(
element = orderByCol,
message = "SELECT DISTINCT ON expressions must match initial ORDER BY expressions",
)
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
CREATE TABLE person (
id INTEGER PRIMARY KEY,
name TEXT,
created_at TIMESTAMPTZ
);

SELECT DISTINCT ON (name) *
FROM person;

SELECT DISTINCT ON (name) *
FROM person
ORDER BY name, created_at DESC;

SELECT DISTINCT ON (id, name) id, name
FROM person
ORDER BY name DESC;

SELECT DISTINCT ON (name, id) id, name, created_at
FROM person
ORDER BY id DESC;

SELECT DISTINCT ON (name, id) id, name
FROM person
ORDER BY id, name ASC;

SELECT DISTINCT ON (name, id) id, name
FROM person
ORDER BY id, name, created_at ASC;

-- fail
SELECT DISTINCT ON (name) *
FROM person
ORDER BY created_at DESC;

-- fail
SELECT DISTINCT ON (name, created_at) id, name, created_at
FROM person
ORDER BY id, name, created_at DESC;

-- fail
SELECT DISTINCT ON (name, id) id, name, created_at
FROM person
ORDER BY name, created_at, id DESC;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Test.s line 33:9 - SELECT DISTINCT ON expressions must match initial ORDER BY expressions
Test.s line 38:9 - SELECT DISTINCT ON expressions must match initial ORDER BY expressions
Test.s line 43:15 - SELECT DISTINCT ON expressions must match initial ORDER BY expressions

0 comments on commit 8597faf

Please sign in to comment.