2010YOUY01 commented on code in PR #16325:
URL: https://github.com/apache/datafusion/pull/16325#discussion_r2135088072


##########
datafusion/sql/src/select.rs:
##########
@@ -77,11 +82,29 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
         }
 
         // Process `from` clause
-        let plan = self.plan_from_tables(select.from, planner_context)?;
+        let plan = self.plan_from_tables(select.from.clone(), 
planner_context)?;
         let empty_from = matches!(plan, LogicalPlan::EmptyRelation(_));
 
         // Process `where` clause
-        let base_plan = self.plan_selection(select.selection, plan, 
planner_context)?;
+        let mut base_plan =

Review Comment:
   I feel we'd better do this rewrite in a separate logical optimizer rule, to 
keep the planning code clean. It can be done with a follow-up PR before adding 
more functionality to scan sampling.
   (Unless there's a specific reason to do this during the planning phase — I 
did notice some rewrites happening there, but I'm not sure why.)



##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4714,3 +4714,115 @@ fn test_using_join_wildcard_schema() {
         ]
     );
 }
+
+#[test]

Review Comment:
   Regarding test structure, I suggest:
   1. Move all of the `sql_integration` tests to `sqllogictest`, since `.slt`s 
are easier to maintain.
   To only show logical plans, you can use
   ```
   set datafusion.explain.logical_plan_only = true;
   
   # sqllogictest tests
   
   # cleanup
   set datafusion.explain.logical_plan_only = true;
   ```
   2. Create a separate `.slt` file for all tests related to `TABLESAMPLE`
   
   To improve test coverage, I recommend to add the following test cases
   1. Select from multiple table, and test only some of table with sample / all 
of the tables have sample.
   2. Test all sample methods in
   
https://github.com/apache/datafusion-sqlparser-rs/blob/84c3a1b325c39c879b68ab712e3b9b3e3e40ed56/src/ast/query.rs#L1475
   and expect error for unimplemented ones.



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