Skip to content

Conversation

kdubovikov
Copy link

This is a proposed fix for #1574. I am unsure if this change breaks any tests because I don't have a correctly setup postgres instance to generate pg_catalog correctly with all extensions. However, I have checked that this change fixes the bug.

@kdubovikov
Copy link
Author

I haven't found any docs that mention what postgres instance I should use to generate pg_catalog. This is needed to complete the pull request and fix tests. Could anyone direct me on where to look?

p.proargnames,
p.proargnames[p.pronargs-p.pronargdefaults+1:p.pronargs]
p.proargnames[p.pronargs-p.pronargdefaults+1:p.pronargs],
CASE WHEN p.prokind = 'a' THEN TRUE ELSE FALSE END
Copy link
Contributor

@ryan-berger ryan-berger Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in the discussion we were having, but I thought it is worth mentioning here.

We probably should do a CASE WHEN p.prokind = 'a' AND p.name != 'COUNT ....' since COUNT is an exception to the nullability rules.

Also, I don't think there needs to be a CASE since you should just be able to use p.prokind = 'a' AND p.name != 'COUNT' as a boolean expression

@kyleconroy kyleconroy self-requested a review August 29, 2022 06:19
Copy link
Collaborator

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sqlc-pg-gen has been updated since this pull request was open. Can you rebase on main and see if you're still seeing this issue?

}
cols = append(cols, &Column{
Name: name,
DataType: columns[0].DataType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that unfortunately the fix is a lot more complex than this, this is saying that any function with more than 0 args the return type should the type of its first arg?

This is true for most aggregates, (min, max, avg, every) but is not generally true of any function call - in particular count, array_agg and probably any user-defined aggregate.

https://www.postgresql.org/docs/15/functions-aggregate.html

I think special cases for these common aggregates is probably a useful change.

@kyleconroy kyleconroy closed this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants