fix: Fix nullability of logical InSubquery expression - #23429
Conversation
github-actions
Bot
added
logical-expr
AdamGS
changed the title
InSubquery expression
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23429 +/- ##
=======================================
Coverage 80.91% 80.91%
=======================================
Files 1103 1103
Lines 377134 377205 +71
Branches 377134 377205 +71
=======================================
+ Hits 305155 305214 +59
- Misses 53787 53793 +6
- Partials 18192 18198 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @AdamGS for working on this. Just had one comment for your consideration.
| Expr::InSubquery(InSubquery { expr, .. }) => expr.nullable(input_schema), | ||
| Expr::InSubquery(InSubquery { expr, subquery, .. }) => { | ||
| let expr_nullable = expr.nullable(input_schema)?; | ||
| let subquery_nullable = subquery.subquery.schema().field(0).is_nullable(); |
There was a problem hiding this comment.
Could we validate that the subquery has exactly one field or use checked field access and return an error here? LogicalPlanBuilder empty can create a zero-field subquery, while the current invariant only rejects more than one field.
There was a problem hiding this comment.
good catch! I'll try and read through this and potentially change the comment on InSubquery::subquery which is why I assumed there is exactly one here.
There was a problem hiding this comment.
pushed a version that uses checked field access.
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Looks good. Left two comments.
| Expr::InSubquery(InSubquery { expr, subquery, .. }) => { | ||
| let expr_nullable = expr.nullable(input_schema)?; | ||
| let subquery_nullable = subquery.subquery.schema().fields().first().ok_or_else(|| { | ||
| exec_datafusion_err!("subquery must return exactly one column of data to compare against") |
There was a problem hiding this comment.
Could this return a planning error instead? The invalid subquery is detected while inspecting a logical expression, so reporting it as an execution error seems misleading.
| } | ||
| } | ||
|
|
||
| /// A scan of `t`, whose single column `a` has the given nullability. |
There was a problem hiding this comment.
we can add a test using LogicalPlanBuilder empty false and verify that nullable returns an error
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
3 participants
Which issue does this PR close?
InSubqueryonly take the expression nullability into account #23428.Rationale for this change
Report correct nullability for
InSubquerylogical exprs.What changes are included in this PR?
OR the expression's and the subquery's nullability, more like
ScalarSubquery.Are these changes tested?
InSubquerynullabilityAre there any user-facing changes?
No