Fix: Infer placeholder type from subquery#22436
Conversation
github-actions
Bot
added
logical-expr
| }) => { | ||
| let subquery_schema = subquery.subquery.schema(); | ||
| let column = Expr::Column(Column::new_unqualified( | ||
| subquery_schema.fields()[0].name().clone(), |
There was a problem hiding this comment.
should we verify the contract of len = 1 here?
There was a problem hiding this comment.
Yeah I can add that check. Would we want to throw an error if the length was not 1 or just have it do nothing?
There was a problem hiding this comment.
an error would be better than a panic, certainly
There was a problem hiding this comment.
since you’ll address it i removed from the queue, let me know once you’ve swapped for an error and i’ll add it back
There was a problem hiding this comment.
I added a match to ensure the length is 1 or return an error.
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Looks good overall! Thanks for working on this.
Can we handle SetComparison (ANY/ALL) while we're at it?
Personally I find the unit tests a little unnecesary if we have SLT, but I guess having both matches the convention here.
adriangb
left a comment
adriangb
left a comment
There was a problem hiding this comment.
Thanks, this looks good!
I would personally prefer to open a tracking issue for ANY/ALL and tackle it in another PR. The well defined boundaries of this PR make it easy to review (and revert if there are issues down the road).
3 participants
Which issue does this PR close?
Closes #15979.
Rationale for this change
$1 IN (SELECT ...)left the placeholder untyped becauseinfer_placeholder_typeshad no arm forInSubqueryand madeget_parameter_types()returnNonefor these placeholders.What changes are included in this PR?
Adds the
InSubqueryarm toinfer_placeholder_types, reading the type from the subquery's projected column. Covers bothINandNOT IN.How are these changes tested?
Unit tests for
INandNOT INplaceholder inference, plus end-to-end sqllogictests withPREPARE/EXECUTE.Are there any user-facing changes?
No.