Skip to content
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

Support uncorrelated EXISTS #14474

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 4, 2025

For the purpose of decorrelation, an uncorrelated plan is a unit. No
verification needs to be performed on it.
Extract variable from a long if condition involving a match. Improves
readability.
Handle the unhandled case returning immediately. This adds additional
return point to the function, but removes subsequent if. At the point of
this additional return we know why we bail out (some unhandled
situation), later the None filter could be construed as a true
condition.
@github-actions github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Feb 4, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @findepi -- this looks like a nice improvement to me

datafusion/optimizer/src/decorrelate_predicate_subquery.rs Outdated Show resolved Hide resolved
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: {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was possible (in theory) to have a correlated subuery in Exists (which would access outder columns)

So in this case, the error might be more clear as a not_impl_err! with "Referencing outer columns (correlated queries) not yet supported: {}" or something

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(although I see this check is simply moved around in this PR, not introduced)

It would also be great if you were able to add a (negative) test for such queries

Copy link
Member Author

@findepi findepi Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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 (#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, #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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: above the important negation was missed (fixed now)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check_correlations_in_subquery should not check uncorrelated plan nodes
2 participants