kosiew commented on code in PR #24043:
URL: https://github.com/apache/datafusion/pull/24043#discussion_r3712158987
##########
datafusion/sql/src/expr/order_by.rs:
##########
@@ -109,7 +109,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
))
}
e => {
- self.sql_expr_to_logical_expr(e, order_by_schema,
planner_context)?
+ let expr = self.sql_expr_to_logical_expr(
+ e,
+ order_by_schema,
+ planner_context,
+ )?;
+ let (expr, _) =
expr.infer_placeholder_types(order_by_schema)?;
Review Comment:
Nice catch applying placeholder inference here. I think this introduces a
regression for `DISTINCT ON`, though.
`ORDER BY` expressions are now inferred, but the corresponding `DISTINCT ON`
expressions are still planned through `sql_expr_to_logical_expr` without
inference (`datafusion/sql/src/select.rs:185-195`). Since
`DistinctOn::with_sort_expr` requires structural equality, these expressions no
longer compare equal.
For example:
```sql
SELECT DISTINCT ON (
CASE WHEN age < $1 THEN 'young' ELSE 'old' END
) first_name
FROM person
ORDER BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END;
```
This now fails with `SELECT DISTINCT ON expressions must match initial ORDER
BY expressions`, whereas before this change both expressions were untyped and
matched.
Could we also infer placeholder types for the `DISTINCT ON` expressions
after alias substitution and normalization? It would also be great to add this
query as a regression test.
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -1504,6 +1504,85 @@ fn
select_aggregate_with_group_by_with_having_using_count_star_not_in_select() {
);
}
+/// An expression containing a placeholder, written in both the SELECT list and
+/// the GROUP BY, has to be recognised as one expression the way its literal
+/// equivalent is. Otherwise the columns inside it read as ungrouped, because
the
+/// SELECT list has its placeholder types inferred and the grouping key does
not.
+#[test]
+fn select_aggregate_with_group_by_placeholder_expression() {
Review Comment:
These snapshot tests demonstrate that planning succeeds, which is great. One
small suggestion would be to add a focused structural assertion that `$1` is
inferred as the type of `person.age`. That would make the placeholder inference
contract explicit instead of only verifying it indirectly through expression
equality.
--
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]
