-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add Expr::InSubquery and Expr::ScalarSubquery
#2342
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
Add Expr::InSubquery and Expr::ScalarSubquery
#2342
Conversation
Expr::InSubquery and Expr::ScalarSubqueryExpr::InSubquery and Expr::ScalarSubquery
alamb
left a comment
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.
Looking very nice @andygrove
All aboard the subquery 🚂 !
I had a few stylistic / corner case comments but nothing that can't be addressed after merge as a follow on PR
datafusion/expr/src/expr_schema.rs
Outdated
| Expr::IsNull(_) | ||
| | Expr::IsNotNull(_) | ||
| | Expr::Exists { .. } | ||
| | Expr::InSubquery { .. } => Ok(false), |
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 aIN can be nullable if its exprs are nullable:
alamb=# select null IN (select * from a);
?column?
----------
(1 row)So maybe Expr::InSubquery should return expr.nullable(input_schema) too
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.
Thanks. Fixed.
| subquery, | ||
| negated, | ||
| } => Ok(Expr::InSubquery { | ||
| expr: Box::new(clone_with_replacement(&**nested_expr, replacement_fn)?), |
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.
👍
| | Expr::Exists(_) | ||
| | Expr::Exists { .. } | ||
| | Expr::InSubquery { .. } | ||
| | Expr::ScalarSubquery(_) |
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.
at some point it would be cool to be able to evaluate scalar subqueries at plan time, "but not now" 😆
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
datafusion/expr/src/expr.rs
Outdated
| Expr::InSubquery { | ||
| expr, | ||
| subquery, | ||
| negated, | ||
| } => { | ||
| if *negated { | ||
| write!(f, "{:?} NOT IN ({:?})", expr, subquery) | ||
| } else { | ||
| write!(f, "{:?} IN ({:?})", expr, subquery) | ||
| } | ||
| } |
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.
The same negated: true / negated: false could be applied to Expr::InSubquery as was applied to Expr::Exists
|
🎉 |
Which issue does this PR close?
Part of #2248
Rationale for this change
Add
InSubqueryandScalarSubquerytoExpras a step towards supporting SQL queries that use these expressions.What changes are included in this PR?
InSubqueryandScalarSubquerytoExprAre there any user-facing changes?
Yes, API change