findepi commented on code in PR #14474:
URL: https://github.com/apache/datafusion/pull/14474#discussion_r1942608923
##########
datafusion/expr/src/logical_plan/invariants.rs:
##########
@@ -272,26 +267,34 @@ fn check_inner_plan(inner_plan: &LogicalPlan,
can_contain_outer_ref: bool) -> Re
| JoinType::LeftSemi
| JoinType::LeftAnti
| JoinType::LeftMark => {
- check_inner_plan(left, can_contain_outer_ref)?;
- check_inner_plan(right, false)
+ check_inner_plan(left)?;
+ check_no_outer_references(right)
}
JoinType::Right | JoinType::RightSemi | JoinType::RightAnti => {
- check_inner_plan(left, false)?;
- check_inner_plan(right, can_contain_outer_ref)
+ check_no_outer_references(left)?;
+ check_inner_plan(right)
}
JoinType::Full => {
inner_plan.apply_children(|plan| {
- check_inner_plan(plan, false)?;
+ check_no_outer_references(plan)?;
Ok(TreeNodeRecursion::Continue)
})?;
Ok(())
}
},
LogicalPlan::Extension(_) => Ok(()),
- _ => plan_err!(
- "Unsupported operator in the subquery plan: {}",
+ plan => check_no_outer_references(plan),
+ }
+}
+
+fn check_no_outer_references(inner_plan: &LogicalPlan) -> Result<()> {
+ if inner_plan.contains_outer_reference() {
+ plan_err!(
+ "Accessing outer reference columns is not allowed in the plan: {}",
Review Comment:
> (although I see this check is simply moved around in this PR, not
introduced)
indeed
> It would also be great if you were able to add a (negative) test for such
queries
i think the check in analyzer is unsound, i wouldn't want to spend time on it
this is because decorrelability is a pretty complex thing. we have two
separate classes/files devoted to it, and we're only scratching the surface
(https://github.com/apache/datafusion/issues/11773). I understand the desire is
to produce nice error messages, but the analyzer cannot, in its brevity, know
which query can run and which query cannot. For example,
https://github.com/apache/datafusion/pull/13523 added as allowed operator in
the analyzer, without adding any decorrelation support for it. Thus, the check
was either incorrect before or after the change (or, likely, both).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
