sqlc.embed() generates code that cannot handle NULL items

[PatrLind]

Version

1.18.0

What happened?

I was testing to implement sqlc.embed() in order to simplify my conversions routines from sqlc to protobuf.

I have this query:

-- name: GetChannelLayers :many
SELECT sqlc.embed(t)
FROM unnest(sqlc.narg('channel_layer_ids')::TEXT[]) WITH ORDINALITY o(channel_layer_id, ord)
LEFT JOIN channel_layer AS t ON t.channel_layer_id = o.channel_layer_id::UUID AND t.tenant_id = $1;

The input is a list of IDs that i expect to get from the DB. If the ID is not found I expect to get a NULL row. Unfortunately it seems like the code for the new sqlc.embed() feature doesn't handle this case very well since I get a scan error if I try to query for a non-existent ID.

pgx.ScanArgError: can't scan into dest[0]: cannot scan null into *string

The generated output code:

const getChannelLayers = `-- name: GetChannelLayers :many
SELECT t.tenant_id, t.channel_id, t.channel_layer_id, t.name, t.description, t.created, t.updated, t.removed, t.disabled, t.condition_ref, t.z_index, t.width, t.height, t.pos_x, t.pos_y, t.alignment, t.content_ref
FROM unnest($2::TEXT[]) WITH ORDINALITY o(channel_layer_id, ord)
LEFT JOIN channel_layer AS t ON t.channel_layer_id = o.channel_layer_id::UUID AND t.tenant_id = $1
`

type GetChannelLayersParams struct {
	TenantID        string
	ChannelLayerIds []string
}

type ChannelLayer struct {
	TenantID       string
	ChannelID      uuid.UUID
	ChannelLayerID uuid.UUID
	Name           string
	Description    sql.NullString
	Created        time.Time
	Updated        time.Time
	Removed        sql.NullTime
	Disabled       sql.NullTime
	ConditionRef   sql.NullString
	ZIndex         sql.NullInt32
	Width          sql.NullInt32
	Height         sql.NullInt32
	PosX           sql.NullInt32
	PosY           sql.NullInt32
	Alignment      ChannelLayerAlignment
	ContentRef     sql.NullString
}

type GetChannelLayersRow struct {
	ChannelLayer ChannelLayer
}

func (q *Queries) GetChannelLayers(ctx context.Context, arg GetChannelLayersParams) ([]*GetChannelLayersRow, error) {
	rows, err := q.db.Query(ctx, getChannelLayers, arg.TenantID, arg.ChannelLayerIds)
	if err != nil {
		return nil, err
	}
	defer rows.Close()
	var items []*GetChannelLayersRow
	for rows.Next() {
		var i GetChannelLayersRow
		if err := rows.Scan(
			&i.ChannelLayer.TenantID,
			&i.ChannelLayer.ChannelID,
			&i.ChannelLayer.ChannelLayerID,
			&i.ChannelLayer.Name,
			&i.ChannelLayer.Description,
			&i.ChannelLayer.Created,
			&i.ChannelLayer.Updated,
			&i.ChannelLayer.Removed,
			&i.ChannelLayer.Disabled,
			&i.ChannelLayer.ConditionRef,
			&i.ChannelLayer.ZIndex,
			&i.ChannelLayer.Width,
			&i.ChannelLayer.Height,
			&i.ChannelLayer.PosX,
			&i.ChannelLayer.PosY,
			&i.ChannelLayer.Alignment,
			&i.ChannelLayer.ContentRef,
		); err != nil {
			return nil, err
		}
		items = append(items, &i)
	}
	if err := rows.Err(); err != nil {
		return nil, err
	}
	return items, nil
}

I'm actually not sure how to solve this. I need the LEFT JOIN because reasons...
Perhaps it's impossible to support this case?

Perhaps sqlc can create a temp/hidden type where all fields are NULL-able. Scan the fields using this type and then check if the PRIMARY KEY fields are Valid. If they are valid, convert the type with the NULL-able fields into the normal type. If the fields are not valid then return a nil object pointer for the embedded field.

@nickjackson, any input?

What operating system are you using?

Linux

What database engines are you using?

PostgreSQL

What type of code are you generating?

Go

[bas-ie]

Playground reproduction: https://play.sqlc.dev/p/f7baf03ee43839b8063555ea33f6d94c2f9f8545e14cb7f41406f880eea432d7

I may actually try to fix this, as it also affects my planned use for embed. Looking at the source, I'm still not quite sure how much work is involved making it respect nullable columns. I'll have a think about it.

[nickjackson]

Hi Both,

Left joins were left out of the implementation because the scanning logic is not so straightforward. Passing the additional context through about a left-join column is also tricky.

Perhaps sqlc can create a temp/hidden type where all fields are NULL-able. Scan the fields using this type and then check if the PRIMARY KEY fields are Valid. If they are valid, convert the type with the NULL-able fields into the normal type. If the fields are not valid then return a nil object pointer for the embedded field.

I think most of this idea has legs (temp type with nullable fields, nil object pointer, seem 👍 ), but there are two issues I can see.

1. While I'm sure 99% of the time the PRIMARY KEY is good enough, what about the cases where a user wants to check a different field?
2. What if the user is using a custom scanning type for the primary key field that doesn't have a Valid attribute or none at all? If done incorrectly, the user will end up with a broken package.

Both of those options are solvable, perhaps with config overrides, but it's worth pointing out. This feature requires sqlc to make business logic decisions which it typically stays out of.

I would love to see this implemented, just to make sqlc.embed feature complete. I might even use it, but right now I just work around this by making an additional query in parallel.

I hope this helps.

[bas-ie]

Yeah, sometimes it's a question of "where do you stop?" with a feature like this I guess. 🤔

It'd be tempting to explicitly provide the support if the primary key is available, and bail out to the standard behaviour if not. This would surely increase its utility quite a bit. However, I do wonder if there's an opportunity for the generator logic to be a bit more well-informed about which joins could result in a null embed even if we're not joining on a primary key (for example, always using a nil pointer for a left join).

Alternatively, could there be room for a sqlc.embed(foo, sqlc.nullable) variant? Which would make it declarative, and put the onus on the developer to request it.

[PatrLind]

Good points nickjackson. I also think richchurcher has a point on the ability to configure the embed() command with parameters.

I think a good start could be to write the code such that it is able to detect the cases it cannot support. Right now the code generated doesn't really work if you happen to do unsupported things. If it is possible to detect all the cases it doesn't work, I think it would be easier to extend it to actually make it work, since then we know all about what type of join it is and so forth.
I took a look at the code, and sqlc doesn't seem to know very much about the join itself, just that there is some kind of relationship to another table.

[nickjackson]

Yeah, I have to be honest the implementation (and the whole of sqlc to some extent) is very naive about what a join is - it is just sugar on column selections.

Happy to answer questions and help out where I can, but I don't have the bandwidth to do this myself, nor do I have any authority to suggest the right course of action. (I'm not a maintainer)

[bas-ie]

I'll have a quiet look at what it would take to go down the parameter path... it feels like it would just be a question of choosing a pointer type, but it can't possibly be that simple 😆

[bas-ie]

I think, having done more poking around, that even if piggybacking on embed might be slightly easier, introducing nembed (nullable embed) is more in line with the existence of arg/narg. From the looks of it, it'll require a bit of a refactor but maybe I can learn from @skabbes' hard work in #1536 .