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
