-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add CREATE VIEW
#2279
Add CREATE VIEW
#2279
Conversation
My idea here is to create a new |
Thanks @matthewmturner -- I'll try and give it a look tomorrow |
@alamb I actually hadn't even made it to the point of implementing that yet - just wanted to see if conceptually you thought that was the right approach. Wasn't expecting you to review anything yet. If you aren't sure and it would require you looking into it I can just give it a shot and get back to you. I don't want you to have to do unnecessary work. |
Sorry for the late review @matthewmturner At a high level, I would expect a VIEW to be represented by a query -- maybe as a SQL string / parsed So let's say we have a query like this select sum(y) from T where a > 5 group by a; If
If create view T as select a, y from bar where y > 10000 I would expect the plan to look like:
The question then becomes how do you want to get the But then again, I am not sure a view makes sense in the context of a dataframe -- the user would just clone the DataFrame (which is a wrapper around a |
I apologize I have been quite busy lately and havent been able to continue my efforts on this. Im really hoping to get this in for 8.0 release, I will try to work on this over the weekend. |
No worries at all - I totally understand! |
@alamb would you mind checking this out and see if its in the right direction? I still have to resolve conflicts and update ballista but wanted to make sure this was at least getting close before proceeding. |
@andygrove FYI I'm hoping to get this in for 8.0 release. ill be continuing my work on it tonight, |
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 this is looking very much on the right track @matthewmturner 👍
/// An implementation of `TableProvider` that uses the object store | ||
/// or file system listing capability to get the list of files. |
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.
The implementation uses another LogicalPlan
filters: &[Expr], | ||
limit: Option<usize>, | ||
) -> Result<Arc<dyn ExecutionPlan>> { | ||
self.context.create_physical_plan(&self.logical_plan).await |
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.
👍
|
||
assert_batches_eq!(expected, &results); | ||
|
||
let results = session_ctx |
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 more interesting tests might be to create a view and then run queries against the view (also adding things like filters to on the view
For example
select coumn1, count(colum2) from xyz where column3 > 2
I just took a quick look through this and LGTM ❤️ |
48c4349
to
0ba010f
Compare
/// To create ExecutionPlan | ||
context: SessionContext, | ||
/// LogicalPlan of the view | ||
logical_plan: LogicalPlan, |
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.
Original SQL might be useful too to store, to preserve formatting when doing things like describe view
.
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.
Indeed that would be nice, do you think we could do it as a follow on PR?
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 so
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.
hi, I'm new to datafusion and maybe I can help with the follow work ?
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.
👋 welcome @Veeupup . That would be great. I will try and write up a ticket later today that describes the work in some more detail in case that is helpful
Thank you!
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.
@Dandandan not sure if you had specific ideas on what command you wanted to show view definitions.
335be74
to
53238a1
Compare
Ah - i believe the tpch tests use a published version of ballista. so i dont think we can run Q15 until our next release. let me know if im misunderstanding, i didnt have time to dig too far into it. |
q15 was definitely a stretch goal for this PR 😄 I will take a look |
RAT is failing due to an empty file @ datafusion/core/src/physical_plan/view.rs |
@andygrove thanks - i removed. |
@andygrove @alamb hopefully this is good now. was i able to complete it in time for the 8.0 release? |
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.
LGTM. Thanks @matthewmturner
@Igosuki FYI |
Which issue does this PR close?
Closes #1740
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?