Skip to content

fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) - #24043

Open
Braedon-Wooding-Displayr wants to merge 1 commit into
apache:mainfrom
Braedon-Wooding-Displayr:placeholder-types-in-group-by
Open

fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042)#24043
Braedon-Wooding-Displayr wants to merge 1 commit into
apache:mainfrom
Braedon-Wooding-Displayr:placeholder-types-in-group-by

Conversation

@Braedon-Wooding-Displayr

Copy link
Copy Markdown

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.

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.
@github-actions github-actions Bot added the sql SQL Planner label Aug 1, 2026
@codecov-commenter

codecov-commenter commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.85%. Comparing base (f2b4835) to head (c25ee4d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/sql/src/select.rs 50.00% 0 Missing and 5 partials ⚠️
datafusion/sql/src/expr/order_by.rs 71.42% 1 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Braedon-Wooding-Displayr

Copy link
Copy Markdown
Author

Most of the missing coverage seems to just be error propagation? Which should be okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GROUP BY expression containing a placeholder does not match the same expression in the SELECT list

2 participants