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

add filter clause support at AggregateFunctionBuilder. #208

Merged
merged 13 commits into from
Dec 15, 2022

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Oct 28, 2022

brought up by @thelinuxlich on discord.

Postgres [1] [2] & SQLite support filter clause when using aggregate functions, with syntax as follows:

aggregate_function(...) [filter(where ...)] [over(...)]

Its a simple alternative to using case...when...then...end inside the aggregate function.

This PR adds filterWhere, filterWhereExists, filterWhereNotExists, filterWhereRef, orFilterWhere, orFilterWhereExists, orFilterWhereNotExists & orFiltereWhereRef to AggregateFunctionBuilder.


  • implement.
  • unit tests.
  • typings tests.
  • ts docs.

Some things to consider..

  • since where is nested in filter, maybe its more intuitive and future-proof to make a FilterBuilder that implements WhereInterface, and make filter accept a FilterBuilderCallback instead of acting as the where itself?
  • maybe we should rename filter to filterWhere to not hide where from caller? went with it, feels more aligned with Kysely's API philosophy.
  • since filter clause is a thing, maybe we should rename the existing FilterNode, parsers, etc and introduce a new FilterNode?

@igalklebanov igalklebanov added api Related to library's API enhancement New feature or request labels Oct 28, 2022
@igalklebanov igalklebanov changed the title add filter clause at AggregateFunctionBuilder. add filter clause support at AggregateFunctionBuilder. Oct 28, 2022
@koskimas
Copy link
Member

koskimas commented Nov 5, 2022

since where is nested in filter, maybe its more intuitive and future-proof to make a FilterBuilder that implements WhereInterface, and make filter accept a FilterBuilderCallback instead of acting as the where itself?

🤔 Maybe. On the other hand, too many nested callbacks starts to look messy.

since filter clause is a thing, maybe we should rename the existing FilterNode, parsers, etc and introduce a new FilterNode?

We could go towards more generic ASTish nodes and rename it to BinaryOperationNode and also have UnaryOperationNode for exists etc. unary operations. We'll need these anyway if we implement the full expression builder at some point. What do you think?

We already have these "semantic" parent nodes like WhereNode and OnNode so we wouldn't lose any "transformability".

I originally called the node tree AST but renamed it to "operation nodes" since they are not actually SQL AST, but kinda like "kysely query builder AST" or something 😄

@igalklebanov
Copy link
Member Author

igalklebanov commented Nov 5, 2022

We could go towards more generic ASTish nodes and rename it to BinaryOperationNode and also have UnaryOperationNode for exists etc. unary operations. We'll need these anyway if we implement the full expression builder at some point. What do you think?

We already have these "semantic" parent nodes like WhereNode and OnNode so we wouldn't lose any "transformability".

Sounds good!

I originally called the node tree AST but renamed it to "operation nodes" since they are not actually SQL AST, but kinda like "kysely query builder AST" or something 😄

😆 KyselyAST™️ has a nice ring to it!

@igalklebanov igalklebanov marked this pull request as ready for review November 25, 2022 10:35
@igalklebanov igalklebanov added postgres Related to PostgreSQL sqlite Related to sqlite labels Nov 25, 2022
@koskimas koskimas force-pushed the window-function-filter branch from 29edd74 to 6462c36 Compare December 15, 2022 07:17
@koskimas koskimas merged commit 72b0054 into kysely-org:master Dec 15, 2022
@igalklebanov igalklebanov deleted the window-function-filter branch June 8, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request postgres Related to PostgreSQL sqlite Related to sqlite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants