Unparse TableScan with projections, filters or fetch to SQL string #12158
Conversation
github-actions
bot
added
sql
datafusion/sql/src/unparser/plan.rs
Outdated
| // TODO: support filters for table scan with alias | ||
| if alias.is_some() && !table_scan.filters().is_empty() { | ||
| return not_impl_err!( | ||
| "Subquery alias is not supported for table scan with pushdown filters" | ||
| ); | ||
| } |
There was a problem hiding this comment.
There are some issues with handling the table name prefix for the column. It isn't simple. I think we can create another issue for it. See the example in the unit tests.
devinjdangelo
left a comment
devinjdangelo
left a comment
There was a problem hiding this comment.
Thanks @goldmedal! This looks reasonable to me. The only possible controversial change I notice is the added public methods in the expr crate. They seem reasonable enough to me though.
Thanks, @devinjdangelo. I removed the controversial parts to align with the coding style. I think they might not be necessary for this PR's purpose. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @goldmedal and @devinjdangelo
I took a look at this PR and it looks good from my point of view. The PR has a conflict that needs to be resolved prior to merging.
cc @phillipleblanc as I think you have been reviewing code in this area as well
| .map(Self::new) | ||
| } | ||
|
|
||
| /// Convert a table provider into a builder with a TableScan with filter and fetch |
There was a problem hiding this comment.
This seems like a reasonably helpful API in my opinion
| "SELECT * FROM t1 WHERE ((t1.id > 1) AND (t1.age < 2))" | ||
| ); | ||
|
|
||
| // TODO: support filters for table scan with alias |
There was a problem hiding this comment.
I suggest we file a ticket for this gap and leave a link to it in this comment
phillipleblanc
left a comment
phillipleblanc
left a comment
There was a problem hiding this comment.
This PR makes sense to me. Thanks!
datafusion/sql/src/unparser/plan.rs
Outdated
| ) -> Result<LogicalPlan> { | ||
| match plan { | ||
| LogicalPlan::TableScan(table_scan) => { | ||
| // TODO: support filters for table scan with alias.Remove this check after #12368 issue. |
There was a problem hiding this comment.
| // TODO: support filters for table scan with alias.Remove this check after #12368 issue. | |
| // TODO: support filters for table scan with alias. Remove this check after #12368 issue. |
|
🚀 thanks @goldmedal for the code and @devinjdangelo and @phillipleblanc for the review |
|
Thanks @devinjdangelo @alamb @phillipleblanc for the review. |
4 participants
Which issue does this PR close?
Closes #12154.
Rationale for this change
Besides
projections, I also handle thefiltersandfetchfor TableScan.What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
no