-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor: Move view and stream from datasource
to catalog
, deprecate View::try_new
#15260
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
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.
Looks all good!
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.
Thanks @logan-keede -- this looks really nice
I have a few questions / comments about avoiding the datafusion-optimizer
dependency -- let me know what you think
datafusion/catalog/Cargo.toml
Outdated
datafusion-execution = { workspace = true } | ||
datafusion-expr = { workspace = true } | ||
datafusion-optimizer = { workspace = true } |
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.
I think it would be really nice to avoid the new dependency on the datafusion-optimizer crate here --
However, it seems like it is related to the fact that coercion is part of the optimizer crate 🤔
I can think of two ways to avoid this:
- Make the caller of
ViewTable::try_new
callcoerce_types
directly - Move the coercion into a new crate (seems overkill)
But the more I think about this the less of a problem I think it is
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.
I have moved it to the only caller SessionContext
, though I have a concern if there is a use case where someone might not want to use SessionContext
but use ViewTable
directly. since they will not get default Coercion now.
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.
We can perhaps add a note to the upgrade guide
The other thing we could do is add a new function and deprecate the old one, so the compiler would tell all users when they had upgrade 🤔 I will try and give this a shot
datasource
to catalog
datasource
to catalog
, deprecate View::try_new
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.
Thank you @logan-keede -- I pushed a commit to deprecate View::try_new
and renamed the apply_required_rule
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.
Thank you
Which issue does this PR close?
datafusion
crate (datafusion/core
) #14444.Rationale for this change
view and stream are
TableProvider
s, catalog seems like the proper place for them considering their Table Providers and their dependency.What changes are included in this PR?
just refactor of both.
I left test view at its original place for now.
Are these changes tested?
Testing by Github CI.
Are there any user-facing changes?
Nope, I have kept backward compatibility.