sql/parser: support nullable arguments in builtin functions

[justinj]

Was a mostly mechanical rebase of #13031. Healthy amounts of 🔬🐶 here.

Also simplified checkReturn a little to return the typedExprs instead of s.

[cockroach-teamcity]

This change is 

[justinj]

Hold off on reviewing this for now, actually, I have some stuff to figure out.

[justinj]

OK, after sleeping on this a bit and discussing it with Jordan, I have a better understanding and I've updated it a bit, please take a look when you get a chance.

[jordanlewis]

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/parser/overload.go, line 541 at r2 (raw file):

func checkReturn(
	ctx *SemaContext, s typeCheckOverloadState,
) ([]TypedExpr, []overloadImpl, bool, error) {

Hmm I'm surprised this works. Although this function doesn't mutate s, defaultTypeCheck does - and since we don't return the resultant s, I think the caller of this function will lose the modifications potentially made to it by defaultTypeCheck if you don't either return s or change everything to pass it as a pointer. It's suspicious that there's no tests that notice this behavior so maybe I'm wrong.

---

Comments from Reviewable

[knz]

LGTM modulo resolution of the open question

---

Reviewed 8 of 8 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/parser/overload.go, line 541 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Hmm I'm surprised this works. Although this function doesn't mutate s, defaultTypeCheck does - and since we don't return the resultant s, I think the caller of this function will lose the modifications potentially made to it by defaultTypeCheck if you don't either return s or change everything to pass it as a pointer. It's suspicious that there's no tests that notice this behavior so maybe I'm wrong.

same question here

---

Comments from Reviewable

[justinj]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/parser/overload.go, line 541 at r2 (raw file):

Previously, knz (kena) wrote…

same question here

Any time checkReturn returns true (which it does in both paths where s is mutated) it's a signal to typeCheckOverloadedExprs to return immediately (which it does, and so s is discarded). So while this is correct, it's certainly a bit confusing. I'm not sure how to improve it without a larger scale refactoring, since checkReturn uses a lot of info from s. I could add a comment but I'm not sure that that would improve the clarity unless you're comparing this to the previous revision (but I might be blinded by having spent a couple days in this code).

One option might be to do the switch outside the calls to this function and then it can be clearer in which cases it's causing a return and in which cases it's not, but this would make the calling function a bit noisy. I don't want to revert this to pass s around, because I think that just obscures what's going on here.

---

Comments from Reviewable

[knz]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/parser/overload.go, line 541 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Any time checkReturn returns true (which it does in both paths where s is mutated) it's a signal to typeCheckOverloadedExprs to return immediately (which it does, and so s is discarded). So while this is correct, it's certainly a bit confusing. I'm not sure how to improve it without a larger scale refactoring, since checkReturn uses a lot of info from s. I could add a comment but I'm not sure that that would improve the clarity unless you're comparing this to the previous revision (but I might be blinded by having spent a couple days in this code).

One option might be to do the switch outside the calls to this function and then it can be clearer in which cases it's causing a return and in which cases it's not, but this would make the calling function a bit noisy. I don't want to revert this to pass s around, because I think that just obscures what's going on here.

Can you extend the comment of this function with your explanation, then we're probably good to go.

---

Comments from Reviewable

[justinj]

Review status: 5 of 8 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/parser/overload.go, line 541 at r2 (raw file):

Previously, knz (kena) wrote…

Can you extend the comment of this function with your explanation, then we're probably good to go.

Ok, done. I spent some time yesterday trying to figure out a more holistic change, but I think this will do for now.

---

Comments from Reviewable

[jordanlewis]

---

Reviewed 5 of 8 files at r1, 3 of 3 files at r5, 1 of 1 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[justinj]

Thanks for the reviews!