alamb commented on code in PR #4902:
URL: https://github.com/apache/arrow-datafusion/pull/4902#discussion_r1070108225


##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1180,6 +1180,12 @@ impl DefaultPhysicalPlanner {
                         "Unsupported logical plan: CreateView".to_string(),
                     ))
                 }
+                LogicalPlan::Write(_) => {
+                    // DataFusion is a read-only query engine, but also a 
library, so consumers may implement this
+                    Err(DataFusionError::Internal(
+                        "Unsupported logical plan: CreateView".to_string(),

Review Comment:
   ```suggestion
                           "Unsupported logical plan: Write".to_string(),
   ```



##########
datafusion/sql/src/statement.rs:
##########
@@ -505,6 +533,192 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
         }))
     }
 
+    fn delete_to_plan(
+        &self,
+        table_factor: TableFactor,
+        predicate_expr: Option<Expr>,
+    ) -> Result<LogicalPlan> {
+        let table_name = match &table_factor {
+            TableFactor::Table { name, .. } => name.clone(),
+            _ => Err(DataFusionError::Plan(
+                "Unsupported table type for delete!".to_string(),

Review Comment:
   I think it would help to say what is supported in the message (tables only?) 



##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1490,6 +1500,38 @@ pub struct CreateExternalTable {
     pub options: HashMap<String, String>,
 }
 
+#[derive(Clone)]
+pub enum WriteOp {
+    Insert,
+    Delete,
+    Update,
+    Ctas,

Review Comment:
   I think datafusion already supports create table as select (
   
   ```
   ❯ create table foo as values (1), (2);
   0 rows in set. Query took 0.026 seconds.
   ❯ create table bar as select * from foo;
   0 rows in set. Query took 0.009 seconds.
   ❯ select * from bar;
   +---------+
   | column1 |
   +---------+
   | 1       |
   | 2       |
   +---------+
   ```
   
   Although maybe that is part of datafusion-cli directly



##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -118,6 +118,8 @@ pub enum LogicalPlan {
     SetVariable(SetVariable),
     /// Prepare a statement
     Prepare(Prepare),
+    /// Insert / Update / Delete
+    Write(WriteRel),

Review Comment:
   I wonder if calling this Dml(DmlStatement) rather than `Write` might be a 
more standard term? However, I think this term this may come from Substrait: 
https://substrait.io/relations/logical_relations/#write-operator so that is ok 
too. 
   
   In general I would like to split logical plan up a bit more into "relational 
operators" and "non-relational actions" (like SetVariable, for example) 
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to