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

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

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


##########
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:
   will subquery_alias need similar treatment to have an OwnedTableReference instead of String? or String itself should be just fine?



##########
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:
   use `TableReference::none()` that you introduced?



##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1196,14 +1221,22 @@ pub fn subquery_alias(
 
 /// Create a LogicalPlanBuilder representing a scan of a table with the provided name and schema.
 /// This is mostly used for testing and documentation.
-pub fn table_scan(
-    name: Option<&str>,
+pub fn table_scan<'a>(
+    name: Option<impl Into<TableReference<'a>>>,
     table_schema: &Schema,
     projection: Option<Vec<usize>>,
 ) -> Result<LogicalPlanBuilder> {
+    let table_source = table_source(table_schema);
+    let name = name
+        .map(|n| n.into())
+        .unwrap_or_else(|| OwnedTableReference::bare(UNNAMED_TABLE))

Review Comment:
   maybe `TableReference::bare(UNNAMED_TABLE)` instead since next line is `to_owned_reference()` anyway?



##########
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:
   im curious about why explicit lifetime is needed here?



##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -217,19 +248,13 @@ impl LogicalPlanBuilder {
 
     /// Convert a table provider into a builder with a TableScan
     pub fn scan_with_filters(
-        table_name: impl Into<String>,
+        table_name: impl Into<OwnedTableReference>,
         table_source: Arc<dyn TableSource>,
         projection: Option<Vec<usize>>,
         filters: Vec<Expr>,
     ) -> Result<Self> {
         let table_name = table_name.into();
 
-        if table_name.is_empty() {
-            return Err(DataFusionError::Plan(
-                "table_name cannot be empty".to_string(),
-            ));
-        }

Review Comment:
   does this check need to be added to `TableReference` related methods? didn't consider this before



##########
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:
   ```suggestion
                       let table_reference = TableReference::from(name).to_owned_reference();
                       scan.table_name = table_reference;
   ```
   
   maybe this possibly?



-- 
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