Uh oh!
There was an error while loading. Please reload this page.
Remove dependency from LogicalPlan::TableScan to ExecutionPlan - #2284
Conversation
LogicalPlan::TableScan to ExecutionPlanandygrove
commented
Apr 20, 2022
@matthewmturner@alamb I aborted the large refactor and found a simpler and more practical approach. Let me know what you think. |
alamb
commented
Apr 20, 2022
I plan to review this later today |
alamb
commented
Apr 20, 2022
The last comment from @jimexist during "the great split of datafusion" was is #1762 (comment) -- I think this PR will help |
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thanks @andygrove
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| /// Extract TableProvider from TableSource | ||
| pub fn source_as_provider( |
There was a problem hiding this comment.
Maybe it is worth a comment on TableSource that it is not intended, initially, to be extended outside of DataFusion.
| Temporary, | ||
| } | ||
| /// TableSource |
There was a problem hiding this comment.
It is probably worth some comments here about the rationale for this structure existing -- namely so LogicalPlan doesn't directly rely on physical plans
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…datafusion into table-provider-refactor
andygrove
commented
Apr 20, 2022
Thanks for the review @alamb. I pushed an update with more documentation. Let me know if I missed anything and I can address in one of the following refactor PRs. |
Which issue does this PR close?
Closes#2247
Rationale for this change
We want to remove the dependency from
TableScantoExecutionPlanso that we can move theLogicalPlaninto theexprcrate.What changes are included in this PR?
TableSourcetrait inexprcrate that is referenced fromTableScanDefaultTableSourcestruct in core crate that wrapsTableProviderAre there any user-facing changes?
Yes, at API level