kosiew commented on code in PR #21694:
URL: https://github.com/apache/datafusion/pull/21694#discussion_r3129314627
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -1898,7 +1898,39 @@ fn test_join_with_table_scan_filters() -> Result<()> {
let sql = plan_to_sql(&join_plan_duplicated_filter)?;
assert_snapshot!(
sql,
- @r#"SELECT * FROM left_table AS "left" INNER JOIN right_table ON
"left".id = right_table.id AND (("left".id > 5) AND (("left"."name" LIKE
'some_name' AND (right_table.age > 10)) AND (right_table.age < 11)))"#
+ @r#"SELECT * FROM left_table AS "left" INNER JOIN right_table ON
"left".id = right_table.id AND ("left".id > 5) WHERE "left"."name" LIKE
'some_name' AND (right_table.age > 10) AND (right_table.age < 11)"#
+ );
+
+ // Inner join with a scalar subquery in table_scan_filters. The subquery
filter should appear in WHERE, not in JOIN ON,
Review Comment:
Nice regression test for the scalar subquery case, this is great to see.
Since the behavior change here is broader than just scalar subqueries, would it
be worth adding IN and maybe EXISTS variants as well? That would make the
contract described in the PR more concrete and help protect against future
refactors that might accidentally only handle Expr::ScalarSubquery.
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -683,28 +683,37 @@ impl Unparser<'_> {
let join_filters = if table_scan_filters.is_empty() {
join.filter.clone()
+ } else if join.join_type == JoinType::Inner {
Review Comment:
This reads correctly to me, but the join branch is now juggling a few
responsibilities, like predicate placement and the AND folding logic for outer
joins. It might be worth extracting a small helper here to make things easier
to scan. Something along the lines of pushing extracted filters into WHERE or
combining them for ON could help keep this section focused more on intent than
on the mechanics.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
