sqlc-dev
Add not null to expression sublink
A simple sublink change to resolve a type generation issue.
THIS PR IS NOT THOROUGHLY TESTED AND SHOULD BE REVIEWED WITH A HIGH LEVEL OF SCRUTINY. WHILE IT IS A VERY SMALL CHANGE, I HAVE NO CONSIDERED THE IMPACT OF THIS CHANGE WHATSOEVER.
THIS PR IS NOT THOROUGHLY TESTED AND SHOULD BE REVIEWED WITH A HIGH LEVEL OF SCRUTINY. WHILE IT IS A VERY SMALL CHANGE, I HAVE NO CONSIDERED THE IMPACT OF THIS CHANGE WHATSOEVER.
UNDERSTOOD THANK YOU VERY MUCH FOR YOUR CONTRIBUTION AND I WILL BE VERY CAREFUL MERGING IT IN I HOPE YOU AREN'T OFFENDED IF I DON'T MERGE IT BECAUSE IT MAY BREAK SOME THINGS
The tests failed which means this change isn't likely to be safe. Can you provide a bit more context on why you needed to do this?
It had to do with issue #1208. I saw you had dismissed the issue as invalid, but I found that the issue was in fact valid. It has to do with how a subquery is processed. As pointed out in the issue, the subquery could return null which would mean that the return value of that subquery should also be null. I will have to check the tests to see where it went wrong and update
Okay so I looked at the failed test. The issue is that when we do a subquery with SELECT COUNT, it is not a nullable value but if we do a subquery of (SELECT * FROM x WHERE id = 1 LIMIT 1) AS "name", like in the example, this query is certainly nullable
Okay, so I dug into this problem a little further. Firstly, I was using the term "subquery" incorrectly. A subquery is a legitimate Postgresql term, see here, that returns a boolean value (meaning that it will always return a NOT NULL value). The other possibility, which I was again confusing the terminology, is an aggregate function. This is what is happening in the test where it is returning a COUNT (a non nullable operator). However, aggregate functions actually CAN return null values! Taken from the doc,
In particular, sum of no rows returns null, not zero as one might expect, and array_agg returns null rather than an empty array when there are no input rows
.The problem highlighted in issue #1208 is that they are actually making an entirely new query, that is not a subquery or aggregate, which MUST be nullable despite the table definition because it could always return nothing. I'm not sure how exactly, but I think that there needs to be more logic in the SubLink case to determine some of these nuances.
Edit: I have no idea why i'm so bad at typing today
Apologies for my word vomit. I'm realizing now that the SubLink likely does handle most of those cases correctly because it is just a recursive call. The only case that it misses is when it recursively calls a plain SELECT x.id FROM x query because this will take the nullable value of the field id, but it should be always be nullable despite the value of the field
Since this PR doesn't have any tests, I'm going to go ahead and close it. @WesleyMiller1998 if you're still running into an issue, please open up a bug report with a complete reproduction of the issue. We can then decide on the fix there.