-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add DataFrame::into_view instead of implementing TableProvider (#2659) #4778
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.
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
Conversation
|
I originally thought about resolving this by moving CatalogList off SessionState and onto SessionContext, but this created a huge amount of churn, this seems relatively harmless by comparison. |
|
|
||
| #[async_trait] | ||
| impl TableProvider for DataFrame { | ||
| impl TableProvider for DataFrameTableProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the ref cycle that this change fixing? I think I am missing something here
I see the cycle happens when the dataframe is registered as a table provider on the same context
I think this makes sense to me
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Dandandan as I think he was the one who added this API at first
|
What is the current thinking about this PR? It has conflicts. Shall we polish it up and merge? It seems like the bug is real enough |
|
Benchmark runs are scheduled for baseline = 4bea81b and contender = c5e2594. c5e2594 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2659
Rationale for this change
The previous version introduced a ref-cycle as the
DataFrameowns a copy of theCatalogListthat in turn owns the registeredDataFrameWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?