Support json_agg(column_ref)

[SimonSimCity]

Fixes #1280.

Wow, my first PR in this repo 🤩

If there was something wrong about what I wrote - e.g. if the types are too narrow or wide, please let me know.

Here's what I know about this:

• Postgresql returns an array if there are elements in the linked table.
• Postgresql returns null otherwise.

According to the docs, this method does only exist for Postgresql, and I therefore didn't look into any other of the options.

[vercel]

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	Jan 13, 2025 8:30am

[pkg-pr-new]

Open in Stackblitz • kysely_koa_example

npm i https://pkg.pr.new/kysely-org/kysely@1316

commit: 39976bb

[SimonSimCity]

Since it's likely that people will use this functionality with coalesce, I thought of adding a helper function, similar to the existing jsonArrayFrom. I don't know how you have it in regards to other dialects, because I would only be able to add it for Postgres. Here's an example of how this could be used: #1280 (comment)

[igalklebanov]

Hey 👋

Thanks! 💪

Since it's likely that people will use this functionality with coalesce, I thought of adding a helper function, similar to the existing jsonArrayFrom. I don't know how you have it in regards to other dialects, because I would only be able to add it for Postgres. Here's an example of how this could be used: #1280 (comment)

I'll need to think about the helper. You're right about the usage pattern due to nullability.

1. How would you name such a helper without making stuff confusing (due to the existence of jsonArrayFrom)?
2. Can it be replicated with other dialects? people might get disappointed with other dialects not having something similar.
3. Not that big of a lift compared to other helpers:

(eb) => eb.coalesce(eb.jsonAgg("col"), sql<string[]>`[]`)

vs.

import { someHelper } from 'kysely/helpers/postgres'

(eb) => someHelper(eb.ref("col"))

The existing helpers were born out of a LOT of signals from the community needing something after they ditched some ORM and did not know how to do nested stuff with SQL.

Regardless, this shouldn't be part of this PR.

[igalklebanov]

Why is a RawBuilder returned here and not AggregateFunctionBuilder?

By not returning the latter you're stripping downstream capabilities.

[igalklebanov]

e.g. this is valid SQL:

SELECT name,
  coalesce(json_agg(laptop.model order by laptop.model DESC) filter (where laptop.model != 'Core 2 Duo'), '[]') as models
from employee 
left join laptop on employee.empId = laptop.assignedTo 
group by employee.empId;

https://onecompiler.com/postgresql/435v97hnu

[SimonSimCity]

It's due to my inexperience with this library 😅 Thanks for pointing this out. Should I add a query like this to the tests as well?

[igalklebanov]

It's due to my inexperience with this library 😅

When you're new to a codebase, it's perfectly fine to not get things right the first time.

I'm just asking questions to get a feel for your thought process here.

You either have a good reason for your choice or by asking, I could find something that could be improved, so the next time, it's clearer what the correct choice should be.

Should I add a query like this to the tests as well?

To runtime tests, maybe.

[igalklebanov]

This one doesn't add much value, let's remove it.

Suggested change

	const r6 = await db
	.selectFrom('person')
	.select((eb) => [
	'first_name',
	eb
	.selectFrom('pet')
	.select((eb) => [eb.fn.jsonAgg('pet.owner_id').as('pet_names')])
	.whereRef('pet.owner_id', '=', 'person.id')
	.as('pet_names'),
	])
	.execute()
	expectType<
	{
	first_name: string
	pet_names: number[] | null
	}[]
	>(r6)

[SimonSimCity]

Could be that this typing-wise doesn't make a difference, but to me this is another use-case. It's using the json-agg in a subquery rather than in a joined table. In a joined table you need a groupBy, but in a subquery you don't.

[igalklebanov]

Typings tests are there to ensure we got things right on the type-level. Not to document the SQL spec.

The relationship between a root query and its sub-queries has been thoroughly tested elsewhere.
groupBy doesn't affect anything on the type-level. This test case doesn't add any value here.

Excessive tests mean:

1. more stuff to maintain.
2. more stuff to wait for locally or in CI.

[igalklebanov]

This could really use a unit test.

[SimonSimCity]

Could you assist me in writing a unit test? What unit do you think should be tested though not to make it an integration test?

[igalklebanov]

Could you assist me in writing a unit test? What unit do you think should be tested though not to make it an integration test?

Sure. We could also pair up on this if you want (DM on Discord).

We have a suite for aggregate functions, probably there.