Skip to content

fix: Add additional required expression for natural join #11713

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

@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
# for free to join this conversation on GitHub. Already have an account? # 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