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.

Sign up for GitHub

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

Jump to bottom

Parsing SQL strings to Exprs with the qualified schema #11562

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Parsing SQL strings to Exprs with the qualified schema #11562

merged 2 commits into from
Jul 25, 2024

Conversation

Lordworms
Copy link
Contributor

@Lordworms Lordworms commented Jul 20, 2024 •
edited
Loading

Which issue does this PR close?

Closes #11551

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 sql SQL Planner core Core DataFusion crate labels Jul 20, 2024
@Lordworms Lordworms marked this pull request as ready for review July 20, 2024 22:46
@github-actions github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jul 20, 2024
@goldmedal
Copy link
Contributor

I guess it will close #11551. How about modifying the description to link this issue?

@Lordworms
Copy link
Contributor Author

I guess it will close #11551. How about modifying the description to link this issue?

sure

goldmedal
goldmedal approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@goldmedal goldmedal 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. LGTM

datafusion/sql/src/expr/identifier.rs Outdated
Comment on lines 55 to 61
let is_unnamed_table = match &qualifier {
TableReference::Bare { table } => table.as_ref() == UNNAMED_TABLE,
TableReference::Partial { table, .. } => {
table.as_ref() == UNNAMED_TABLE
}
TableReference::Full { table, .. } => table.as_ref() == UNNAMED_TABLE,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alamb alamb changed the title Parsing SQL strings to Exprs wtih the qualified schema Parsing SQL strings to Exprs with the qualified schema Jul 22, 2024
@alamb alamb requested a review from jonahgao July 22, 2024 18:51
@alamb
Copy link
Contributor

alamb commented Jul 22, 2024

Thanks @Lordworms and @goldmedal

Since this changes the logic to resolve identifiers I think it should have a more careful review. Maybe @jonahgao has time. If not I will try and find time later this week

jonahgao
jonahgao approved these changes Jul 23, 2024
View reviewed changes
Copy link
Member

@jonahgao jonahgao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left some comments, and overall, it looks good to me. I think determining the qualifiers in advance might be an improvement.

Previously, it always returned unqualified columns, possibly because it didn't search within a schema.
Related PR: #1449 #5593

datafusion/sql/src/expr/identifier.rs Show resolved Hide resolved
datafusion/sql/src/expr/identifier.rs Outdated Show resolved Hide resolved
@jonahgao jonahgao merged commit c951824 into apache:main Jul 25, 2024
24 checks passed
@jonahgao
Copy link
Member

Thanks @Lordworms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goldmedal goldmedal goldmedal approved these changes

@jonahgao jonahgao jonahgao approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parsing SQL strings to Exprs wtih the qualified schema
4 participants
@Lordworms @goldmedal @alamb @jonahgao