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