-
Notifications
You must be signed in to change notification settings - Fork 802
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
fix: #1574 nullable return types for groupby functions #1631
Conversation
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? |
@@ -23,7 +23,8 @@ SELECT p.proname as name, | |||
format_type(p.prorettype, NULL), | |||
array(select format_type(unnest(p.proargtypes), NULL)), | |||
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 |
There was a problem hiding this comment.
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
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
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.