alamb commented on code in PR #14631:
URL: https://github.com/apache/datafusion/pull/14631#discussion_r1953445303


##########
datafusion/core/src/datasource/memory.rs:
##########
@@ -648,9 +649,14 @@ mod tests {
         // Create a table scan logical plan to read from the source table
         let scan_plan = LogicalPlanBuilder::scan("source", source, 
None)?.build()?;
         // Create an insert plan to insert the source data into the initial 
table
-        let insert_into_table =
-            LogicalPlanBuilder::insert_into(scan_plan, "t", &schema, 
InsertOp::Append)?
-                .build()?;
+        let insert_into_table = LogicalPlanBuilder::insert_into(
+            scan_plan,
+            "t",
+            table_sink,
+            &schema,

Review Comment:
   Given the table_source already has the target schema, we could avoid storing 
it again



##########
datafusion/expr/src/logical_plan/dml.rs:
##########
@@ -91,10 +91,12 @@ impl Hash for CopyTo {
 
 /// The operator that modifies the content of a database (adapted from
 /// substrait WriteRel)
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Clone)]
 pub struct DmlStatement {
     /// The table name
     pub table_name: TableReference,
+    /// this is sink table which corresponds to table_name reference

Review Comment:
   ```suggestion
       /// this is target table to insert into
   ```



##########
datafusion/core/src/datasource/memory.rs:
##########
@@ -648,9 +649,14 @@ mod tests {
         // Create a table scan logical plan to read from the source table
         let scan_plan = LogicalPlanBuilder::scan("source", source, 
None)?.build()?;
         // Create an insert plan to insert the source data into the initial 
table
-        let insert_into_table =
-            LogicalPlanBuilder::insert_into(scan_plan, "t", &schema, 
InsertOp::Append)?
-                .build()?;
+        let insert_into_table = LogicalPlanBuilder::insert_into(
+            scan_plan,
+            "t",
+            table_sink,

Review Comment:
   I found the name of `table_sink` to be confusing here (as the type is called 
`TableSource` I would expect it to be called `table_source` for consistency)



##########
datafusion/core/src/datasource/memory.rs:
##########
@@ -648,9 +649,14 @@ mod tests {
         // Create a table scan logical plan to read from the source table
         let scan_plan = LogicalPlanBuilder::scan("source", source, 
None)?.build()?;
         // Create an insert plan to insert the source data into the initial 
table
-        let insert_into_table =
-            LogicalPlanBuilder::insert_into(scan_plan, "t", &schema, 
InsertOp::Append)?
-                .build()?;
+        let insert_into_table = LogicalPlanBuilder::insert_into(
+            scan_plan,
+            "t",
+            table_sink,

Review Comment:
   But now I see that it is a sink because it is the target of the DML 
operations
   
   Maybe `target` would be clearer 🤔 



-- 
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: github-unsubscr...@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to