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 within group clause support for aggregate function builder #1024

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

ivashog
Copy link
Contributor

@ivashog ivashog commented Jun 5, 2024

closes #781

Dev K0te and others added 18 commits March 7, 2024 11:08
Copy link

vercel bot commented Jun 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2024 11:20pm

@ivashog ivashog changed the title Add within group clause support for aggregate functions Add within group clause support for aggregate function builder Jun 5, 2024
@ivashog
Copy link
Contributor Author

ivashog commented Jun 5, 2024

@igalklebanov, I want to consult about the API of the future within group () functionality.
I see the following possible varaints:

  1. Extended - fn.agg('mode').withinGroup(wg => wg.orderBy('field')):
    + : looks like SQL
    - : too verbose; need to create separate WithingGroupBuilder
  2. Simplified - fn.agg('mode').withinGroup('field'), because within group () use only order by expression
  3. Also I can try to implement both variants in one method

@igalklebanov igalklebanov added enhancement New feature or request postgres Related to PostgreSQL api Related to library's API labels Jun 10, 2024
@igalklebanov igalklebanov added mssql Related to MS SQL Server (MSSQL) oracle Related to Oracle labels Jun 10, 2024
@igalklebanov
Copy link
Member

igalklebanov commented Jun 10, 2024

@igalklebanov, I want to consult about the API of the future within group () functionality. I see the following possible varaints:

  1. Extended - fn.agg('mode').withinGroup(wg => wg.orderBy('field')):
    + : looks like SQL
    - : too verbose; need to create separate WithingGroupBuilder
  2. Simplified - fn.agg('mode').withinGroup('field'), because within group () use only order by expression
  3. Also I can try to implement both variants in one method

Hey 👋

We had a similar situation with filter (where ...). All current SQL specs only support where clause there, so we decided, for simplicity, to just go with filterWhere(...).

So I'd go with withinGroupOrderBy(...) for now. WDYT?

@ivashog
Copy link
Contributor Author

ivashog commented Jun 10, 2024

Hey 👋

We had a similar situation with filter (where ...). All current SQL specs only support where clause there, so we decided, for simplicity, to just go with filterWhere(...).

So I'd go with withinGroupOrderBy(...) for now. WDYT?

OK! We think in the same direction)

c81d766#diff-0e4fd806a08c99c93c0331a3d0e75d4c726e4face483d2e21a9febe03e944593R167

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 mssql Related to MS SQL Server (MSSQL) oracle Related to Oracle postgres Related to PostgreSQL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add within group (order by ...) clause support for aggregate functions.
2 participants