fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) - #24043
Conversation
The SELECT list is planned by sql_to_expr, which infers placeholder types. These four clauses are planned by sql_expr_to_logical_expr, which does not, so the same expression written in both places does not compare equal. The result is that a grouping key containing a placeholder is never matched against the identical SELECT expression, and the columns inside it are reported as ungrouped. The same query with literals in place of the placeholder plans fine. QUALIFY fails differently, on a duplicate field name, because the typed and untyped spellings print alike but are not equal. Adds a planner test per clause.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24043 +/- ##
==========================================
- Coverage 80.85% 80.85% -0.01%
==========================================
Files 1101 1101
Lines 375467 375481 +14
Branches 375467 375481 +14
==========================================
+ Hits 303596 303600 +4
- Misses 53777 53778 +1
- Partials 18094 18103 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Most of the missing coverage seems to just be error propagation? Which should be okay. |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the fix. I think the overall approach is the right one and the added regression coverage is helpful.
I did find one regression that should be addressed before merging. I also have one non-blocking suggestion that could make the tests more explicit about the placeholder type inference contract.
| order_by_schema, | ||
| planner_context, | ||
| )?; | ||
| let (expr, _) = expr.infer_placeholder_types(order_by_schema)?; |
There was a problem hiding this 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:
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.
| /// 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() { |
There was a problem hiding this 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.
3 participants
The SELECT list is planned by sql_to_expr, which infers placeholder types. These four clauses are planned by sql_expr_to_logical_expr, which does not, so the same expression written in both places does not compare equal.
The result is that a grouping key containing a placeholder is never matched against the identical SELECT expression, and the columns inside it are reported as ungrouped. The same query with literals in place of the placeholder plans fine. QUALIFY fails differently, on a duplicate field name, because the typed and untyped spellings print alike but are not equal.
Adds a planner test per clause.
Which issue does this PR close?
Rationale for this change
I think it's clearly explained above / in issue.
What changes are included in this PR?
New tests for each clause case + inferring types in the clauses.
Are these changes tested?
Yes tests are there.
Are there any user-facing changes?
No.