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

fix: Add additional required expression for natural join #11713

Merged
merged 3 commits into from
Aug 3, 2024

Conversation

Lordworms
Copy link
Contributor

Which issue does this PR close?

Closes #11635

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 30, 2024
@alamb alamb changed the title fix: autocomplete projection expression for natural join fix: Add additional required expression for natural join Aug 1, 2024
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.

Thanks @Lordworms -- this code looks reasonable to me. I had some suggestions / requests for some more comments.

Maybe @jonahgao has ideas of how to improve this too

datafusion/expr/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
datafusion/expr/src/logical_plan/builder.rs Outdated Show resolved Hide resolved
@jonahgao
Copy link
Member

jonahgao commented Aug 2, 2024

I checked that the final logical plan is correct, but the calling of expand_wildcard in wrap_projection_for_join_if_necessary breaks it. expand_wildcard removes duplicate columns in the using join, but this column is needed by the parent plan.

The use of expand_wildcard was introduced in #4443. I speculate that it was a refactor of the following old implementation.

let mut projection = vec![Expr::Wildcard];
        projection.extend(expr_join_keys.into_iter());
        LogicalPlanBuilder::from(input)
            .project(projection)?
            .build()?

Its purpose should be to re-project all the outputs of the input plan.

So I think another simpler fix might be to replace expand_wildcard with this.

@Lordworms
Copy link
Contributor Author

Lordworms commented Aug 2, 2024

I checked that the final logical plan is correct, but the calling of expand_wildcard in wrap_projection_for_join_if_necessary breaks it. expand_wildcard removes duplicate columns in the using join, but this column is needed by the parent plan.

The use of expand_wildcard was introduced in #4443. I speculate that it was a refactor of the following old implementation.

let mut projection = vec![Expr::Wildcard];
        projection.extend(expr_join_keys.into_iter());
        LogicalPlanBuilder::from(input)
            .project(projection)?
            .build()?

Its purpose should be to re-project all the outputs of the input plan.

So I think another simpler fix might be to replace expand_wildcard with this.
expand_wildcard

I agree, It's is much simpler, I was thinking about how to add missing expressions that are excluded by expand_wildcard but haven't thought about replace it directly. Thanks for this.

@github-actions github-actions bot removed the core Core DataFusion crate label Aug 2, 2024
Copy link
Member

@jonahgao jonahgao left a comment

Choose a reason for hiding this comment

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

Looks good to me!

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 @Lordworms and @jonahgao -- this is a beautifully simple PR now. 🏆

@@ -1531,7 +1531,12 @@ pub fn wrap_projection_for_join_if_necessary(

let need_project = join_keys.iter().any(|key| !matches!(key, Expr::Column(_)));
let plan = if need_project {
let mut projection = expand_wildcard(input_schema, &input, None)?;
// Include all columns from the input and extend them with the join keys
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 that is beautiful.

@alamb alamb merged commit 9e90e17 into apache:main Aug 3, 2024
24 checks passed
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 sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A valid SQL query returned 'Schema error: ...' (SQLancer)
3 participants