[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5615: Use TableReference for TableScan

["alamb (via GitHub)" <gi...@apache.org>]

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


##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2388,12 +2389,13 @@ mod tests {
     }
 
     fn test_plan() -> LogicalPlan {
+        let table_ref: Option<TableReference<'_>> = None;

Review Comment:
   Excellent call. Done in 2c3b6d7e6



##########
datafusion/optimizer/src/inline_table_scan.rs:
##########
@@ -57,7 +57,7 @@ impl OptimizerRule for InlineTableScan {
                         generate_projection_expr(projection, sub_plan)?;
                     let plan = LogicalPlanBuilder::from(sub_plan.clone())
                         .project(projection_exprs)?
-                        .alias(table_name)?;
+                        .alias(table_name.to_string())?;

Review Comment:
   I think it is actually important that the subquery alias is a non parsed string here  (otherwise tests fail). 
   
   This rewrite is effectively inlining the view defition as a named subquery (that doesn't make sense to be in a schema):
   
   ```sql
   ...
   FROM <view definition> as "table_name"
   ```
   
   I will add a comment here. 



##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -2518,7 +2518,8 @@ Internal error: Optimizer rule 'type_coercion' failed due to unexpected error: E
             match ctx.read_csv(path, options).await?.into_optimized_plan()? {
                 LogicalPlan::TableScan(ref scan) => {
                     let mut scan = scan.clone();
-                    scan.table_name = name.to_string();
+                    let table_reference: TableReference<'_> = name.into();
+                    scan.table_name = table_reference.to_owned_reference();

Review Comment:
   That looks much better -- thank you 👍 



##########
datafusion/common/src/dfschema.rs:
##########
@@ -662,12 +666,12 @@ impl DFField {
     }
 
     /// Create a qualified field from an existing Arrow field
-    pub fn from_qualified(
-        qualifier: impl Into<OwnedTableReference>,
+    pub fn from_qualified<'a>(
+        qualifier: impl Into<TableReference<'a>>,

Review Comment:
   Basically because the compiler told me to do so 😆 
   
   But seriously, I think the reason is that `impl` doesn't support anonymous lifetimes -- I would have to change this to use the explicit generic syntax
   
   Here is what happens if I remove `<'a>`:
   
   ```
      --> datafusion/common/src/dfschema.rs:113:45
       |
   113 |         qualifier: impl Into<TableReference<'_>>,
       |                                             ^^ expected named lifetime parameter
       |
   help: consider introducing a named lifetime parameter
       |
   112 ~     pub fn try_from_qualified_schema<'a>(
   113 ~         qualifier: impl Into<TableReference<'a>>,
       |
   ````



-- 
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-unsubscribe@arrow.apache.org

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