Improve SQL Planner docs #14669
Merged
Improve SQL Planner docs #14669
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
sql
alamb
commented
Feb 14, 2025
| //! [`SqlToRel`] planner. | ||
| //! 2. The AST is converted to a [`LogicalPlan`] and logical expressions | ||
| //! [`Expr`]s to compute the desired result by [`SqlToRel`]. This phase | ||
| //! also includes name and type resolution ("binding"). |
There was a problem hiding this comment.
I mostly wanted the term "binding" to appear here, but then I got carried away when I explored a bit more
alamb
commented
Feb 14, 2025
jonahgao
approved these changes
Feb 15, 2025
datafusion/core/src/lib.rs
Outdated
| @@ -185,7 +185,7 @@ | |||
| //! specialize any behavior for your use case. For example, | |||
| //! some projects may add custom [`ExecutionPlan`] operators, or create their own | |||
| //! query language that directly creates [`LogicalPlan`] rather than using the | |||
| //! built in SQL planner, [`SqlToRel`]. | |||
| //! provided in SQL planner, [`SqlToRel`]. | |||
datafusion/expr/src/planner.rs
Outdated
| /// CTE stands for "Common Table Expression" | ||
| /// | ||
| /// # Notes | ||
| /// We don't directly implement this in ['SqlToRel`] as implementing this function |
There was a problem hiding this comment.
This paragraph is not well formatted.
datafusion/sql/src/planner.rs
Outdated
| /// phase looks up table and column names using the [`ContextProvider`]. | ||
| /// 2. Mechanical translation of the AST into a [`LogicalPlan`]. | ||
| /// | ||
| /// It does not perform type checking, semantic analysis, type coercion, or |
There was a problem hiding this comment.
I'm not sure if check_unnest_arg conflicts with the description of type checking.
There was a problem hiding this comment.
This is a good point that some non trivial amount of semantic analysis and checking is done in the planner. I removed this text to make it:
/// It does not perform type coercion, or perform optimization, which are done
/// by subsequent passes.
Co-authored-by: Jonah Gao <[email protected]>
github-actions
bot
removed
the
documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
I have been listening to CMU's A Journey Through Database Query Optimization
which has inspired me to improve the documentation on our planner. (🙌 @lmwnshn)
Specifically the term "Binder" comes up a bunch in those lectures which I wasn't familiar with (I have always heard this concept referred to as variable and type resolution). Anyhow I wanted to make sure "binding" appeared in our docs and I saw the somewhat sparse docs and got carried away
What changes are included in this PR?
Improve the docs for
SqlToReland related entitesAre these changes tested?
By docs CI
Are there any user-facing changes?