implement `drop view` #3267

kmitchener · 2022-08-25T17:53:25Z

Which issue does this PR close?

Closes #3251.

Rationale for this change

What changes are included in this PR?

implement drop view and drop view if exists
change drop table so it only works on tables, not views
added tests for the above
added basic documentation for drop view

Are there any user-facing changes?

avantgardnerio · 2022-08-25T18:07:02Z

@DaltonModlin may be interested in this.

codecov-commenter · 2022-08-25T18:34:25Z

Codecov Report

Merging #3267 (77759ff) into master (a49e01e) will decrease coverage by 0.01%.
The diff coverage is 73.56%.

@@            Coverage Diff             @@
##           master    #3267      +/-   ##
==========================================
- Coverage   85.48%   85.47%   -0.02%     
==========================================
  Files         294      294              
  Lines       54072    54130      +58     
==========================================
+ Hits        46223    46267      +44     
- Misses       7849     7863      +14

Impacted Files	Coverage Δ
datafusion/core/src/physical_plan/planner.rs	`77.39% <0.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`77.11% <0.00%> (-0.95%)`	⬇️
datafusion/expr/src/utils.rs	`90.74% <ø> (ø)`
...tafusion/optimizer/src/common_subexpr_eliminate.rs	`92.07% <ø> (ø)`
datafusion/optimizer/src/projection_push_down.rs	`98.06% <ø> (ø)`
datafusion/proto/src/logical_plan.rs	`17.32% <0.00%> (-0.04%)`	⬇️
datafusion/sql/src/planner.rs	`80.34% <40.00%> (-0.13%)`	⬇️
datafusion/core/tests/sql/create_drop.rs	`93.70% <87.87%> (-2.05%)`	⬇️
datafusion/core/src/execution/context.rs	`78.75% <91.17%> (+0.51%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kmitchener · 2022-08-25T19:45:57Z

Functionality plus basic tests and docs added

kmitchener · 2022-08-26T13:30:46Z

One thing I didn't do in this PR: re-export the new datafusion_expr::logical_plan::DropView struct via the "legacy" logical_plan module in core (datafusion::logical_plan:: ..). Should I re-export it or does legacy in this case mean that no new re-exports should happen there?

https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/src/logical_plan/mod.rs#L18

Alternatively, should we create an issue to remove this legacy re-exporting?

andygrove · 2022-08-26T13:35:05Z

Alternatively, should we create an issue to remove this legacy re-exporting?

We do have an issue already for this - #2683

We may want to deprecate this module for one release and then remove in the next.

andygrove · 2022-08-26T13:36:04Z

docs/source/user-guide/sql/ddl.md

+DROP TABLE IF EXISTS nonexistent_table;
+```
+
+## DROP VIEW


Thanks for improving the docs!

docs/source/user-guide/sql/ddl.md

kmitchener · 2022-08-31T01:10:20Z

@andygrove This is ready. Let me know what I can do to help get it committed. Thanks!

kmitchener · 2022-09-02T13:02:29Z

@alamb is there someone that's suitable to review this and give feedback? this PR is blocking progress on other issues and it's not getting a review :( and I'm not sure how to proceed

alamb · 2022-09-03T09:03:20Z

SOrry @kmitchener -- I will review it now

alamb

This looks great @kmitchener -- thank you and I apologize again for the delay in review

alamb · 2022-09-03T09:19:38Z

datafusion/sql/src/planner.rs

+                    schema: DFSchemaRef::new(DFSchema::empty()),
+                })),
+                _ => Err(DataFusionError::NotImplemented(
+                    "Only `DROP TABLE/VIEW  ...` statement is supported currently"


alamb · 2022-09-03T09:21:41Z

datafusion/expr/src/logical_plan/plan.rs

@@ -1019,6 +1030,17 @@ pub struct DropTable {
    pub schema: DFSchemaRef,
 }

+/// Drops a view.


Not for this PR, but it seems to me like requiring new LogicalPlan nodes for each type of DDL (CREATE, DROP, etc) is somewhat cumbersome. Maybe we can eventually refactor type of thing into LogicalPlan::DDL(DDL) and keep all the DDL related structs in their own structure. I'll file a follow on ticket for this

Filed #3349 as a follow on

Yes, I was wondering about this and follow on work for dropping schema, etc. A lot of plumbing here, and I'm not sure it works if you have your own CatalogProvider.

Yes, I was wondering about this and follow on work for dropping schema, etc. A lot of plumbing here, and I'm not sure it works if you have your own CatalogProvider.

I agree -- it does not work if you have your own CatalogProvider without some more work. I this behavior is fine, as long as the point is clear to users.

alamb · 2022-09-03T09:23:11Z

datafusion/core/src/execution/context.rs

-                        let plan = LogicalPlanBuilder::empty(false).build()?;
-                        Ok(Arc::new(DataFrame::new(self.state.clone(), &plan)))
-                    }
+                    (true, false, Ok(_)) => self.return_empty_dataframe(),


This change to refactor and reduce duplicated code, is a very nice example of leaving the code better than when you found it. It is very much appreciated @kmitchener

alamb · 2022-09-03T09:25:09Z

datafusion/core/tests/sql/create_drop.rs

+
+#[tokio::test]
+#[should_panic(expected = "doesn't exist")]
+async fn drop_view_cant_drop_table() {


ursabot · 2022-09-03T09:31:44Z

Benchmark runs are scheduled for baseline = 0fc7297 and contender = 5621e3b. 5621e3b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

* add initial support for 'drop view' * add tests * add very basic docs * add example to drop view doc * ran prettier on the doc * refactored, now done * removed typo * fix bad resolve merge conflict

add initial support for 'drop view'

8b94eeb

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Aug 25, 2022

kmitchener added 2 commits August 25, 2022 15:30

add tests

b6c023e

add very basic docs

f84eecc

kmitchener changed the title ~~implement 'drop view'~~ implement drop view Aug 25, 2022

kmitchener marked this pull request as ready for review August 25, 2022 19:42

kmitchener added 3 commits August 25, 2022 16:16

add example to drop view doc

947e713

ran prettier on the doc

5f3f77e

refactored, now done

b90f722

andygrove reviewed Aug 26, 2022

View reviewed changes

docs/source/user-guide/sql/ddl.md Outdated Show resolved Hide resolved

removed typo

97cbb87

kmitchener mentioned this pull request Aug 26, 2022

Add support for non-correlated subqueries #3266

Closed

This was referenced Aug 26, 2022

TPC-H Query 15 #166

Closed

Execute LogicalPlans after building for TPCH Benchmarks #3273

Closed

kmitchener added 2 commits August 30, 2022 20:14

Merge branch 'master' into 3251-drop-view-1

1f8958f

fix bad resolve merge conflict

7650489

kmitchener requested a review from andygrove August 31, 2022 01:09

Merge branch 'master' into 3251-drop-view-1

77759ff

alamb approved these changes Sep 3, 2022

View reviewed changes

alamb merged commit 5621e3b into apache:master Sep 3, 2022

alamb mentioned this pull request Sep 3, 2022

Consolidate DDL / Catalog manipulation LogicalPlans (refactor) #3349

Closed

kmitchener deleted the 3251-drop-view-1 branch September 4, 2022 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `drop view` #3267

implement `drop view` #3267

kmitchener commented Aug 25, 2022 •

edited

Loading

avantgardnerio commented Aug 25, 2022

codecov-commenter commented Aug 25, 2022 •

edited

Loading

kmitchener commented Aug 25, 2022

kmitchener commented Aug 26, 2022

andygrove commented Aug 26, 2022

andygrove Aug 26, 2022

kmitchener commented Aug 31, 2022

kmitchener commented Sep 2, 2022

alamb commented Sep 3, 2022

alamb left a comment

alamb Sep 3, 2022

alamb Sep 3, 2022

alamb Sep 3, 2022

kmitchener Sep 4, 2022

alamb Sep 4, 2022

alamb Sep 3, 2022

alamb Sep 3, 2022

ursabot commented Sep 3, 2022

implement drop view #3267

implement drop view #3267

Conversation

kmitchener commented Aug 25, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

avantgardnerio commented Aug 25, 2022

codecov-commenter commented Aug 25, 2022 • edited Loading

Codecov Report

kmitchener commented Aug 25, 2022

kmitchener commented Aug 26, 2022

andygrove commented Aug 26, 2022

Choose a reason for hiding this comment

kmitchener commented Aug 31, 2022

kmitchener commented Sep 2, 2022

alamb commented Sep 3, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Sep 3, 2022

implement `drop view` #3267

implement `drop view` #3267

kmitchener commented Aug 25, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading