You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/08/11 20:45:10 UTC
(datafusion) branch gh-readonly-queue/main/pr-24043-61bc3907ed8c841ea7546384554f5d0a57cb52c2 created (now ef181e8e01)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-24043-61bc3907ed8c841ea7546384554f5d0a57cb52c2
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at ef181e8e01 fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) (#24043)
This branch includes the following new commits:
new ef181e8e01 fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) (#24043)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) (#24043)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-24043-61bc3907ed8c841ea7546384554f5d0a57cb52c2
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit ef181e8e01083a430e6e679d61824209c8e67a00
Author: Braedon Wooding <br...@displayr.com>
AuthorDate: Wed Aug 12 06:44:49 2026 +1000
fix: infer placeholder types in GROUP BY, HAVING, QUALIFY and ORDER BY (fix for #24042) (#24043)
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?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
- Closes #24042.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
I think it's clearly explained above / in issue.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
New tests for each clause case + inferring types in the clauses.
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->
Yes tests are there.
## Are there any user-facing changes?
No.
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
---
datafusion/sql/src/expr/order_by.rs | 8 ++-
datafusion/sql/src/select.rs | 16 ++++-
datafusion/sql/tests/sql_integration.rs | 107 ++++++++++++++++++++++++++++++++
3 files changed, 127 insertions(+), 4 deletions(-)
diff --git a/datafusion/sql/src/expr/order_by.rs b/datafusion/sql/src/expr/order_by.rs
index faecfbcfec..0067a1ebd7 100644
--- a/datafusion/sql/src/expr/order_by.rs
+++ b/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)?;
+ expr
}
};
sort_expr_vec.push(make_sort_expr(expr, asc, nulls_first));
diff --git a/datafusion/sql/src/select.rs b/datafusion/sql/src/select.rs
index bdab013144..bbd9d203eb 100644
--- a/datafusion/sql/src/select.rs
+++ b/datafusion/sql/src/select.rs
@@ -191,7 +191,9 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
// ON expression is a bare identifier. `b` resolves to the
// alias; `b + 0` keeps `b` as the input column.
let expr = substitute_top_level_alias(expr, &alias_map);
- normalize_col(expr, &projected_plan)
+ let expr = normalize_col(expr, &projected_plan)?;
+ let (expr, _) = expr.infer_placeholder_types(&on_expr_schema)?;
+ Ok(expr)
})
.collect::<Result<Vec<Expr>>>()?;
@@ -219,7 +221,10 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
// SELECT c1, MAX(c2) AS m FROM t GROUP BY c1 HAVING MAX(c2) > 10;
//
let having_expr = resolve_aliases_to_exprs(having_expr, &alias_map)?;
- normalize_col(having_expr, &projected_plan)
+ let having_expr = normalize_col(having_expr, &projected_plan)?;
+ let (having_expr, _) =
+ having_expr.infer_placeholder_types(&combined_schema)?;
+ Ok(having_expr)
})
.transpose()?;
@@ -248,6 +253,8 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
base_plan.schema(),
std::slice::from_ref(&group_by_expr),
)?;
+ let (group_by_expr, _) =
+ group_by_expr.infer_placeholder_types(&combined_schema)?;
Ok(group_by_expr)
})
.collect::<Result<Vec<Expr>>>()?
@@ -286,7 +293,10 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
// select row_number() over (PARTITION BY id) as rk from users qualify row_number() over (PARTITION BY id) > 1;
//
let qualify_expr = resolve_aliases_to_exprs(qualify_expr, &alias_map)?;
- normalize_col(qualify_expr, &projected_plan)
+ let qualify_expr = normalize_col(qualify_expr, &projected_plan)?;
+ let (qualify_expr, _) =
+ qualify_expr.infer_placeholder_types(&combined_schema)?;
+ Ok(qualify_expr)
})
.transpose()?;
diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs
index 4f282f5e06..08a95381b3 100644
--- a/datafusion/sql/tests/sql_integration.rs
+++ b/datafusion/sql/tests/sql_integration.rs
@@ -1504,6 +1504,113 @@ fn select_aggregate_with_group_by_with_having_using_count_star_not_in_select() {
);
}
+/// Asserts that placeholder `id` (e.g. `"$1"`) was inferred as `expected` type
+/// somewhere in `plan`.
+fn assert_placeholder_type(plan: &LogicalPlan, id: &str, expected: DataType) {
+ let param_types = plan.get_parameter_types().unwrap();
+ assert_eq!(param_types.get(id), Some(&Some(expected)));
+}
+
+/// 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() {
+ let sql = "SELECT CASE WHEN age < $1 THEN 'young' ELSE 'old' END, count(*)
+ FROM person
+ GROUP BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END";
+ let plan = logical_plan(sql).unwrap();
+ assert_snapshot!(
+ plan,
+ @r#"
+ Projection: CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END, count(*)
+ Aggregate: groupBy=[[CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END]], aggr=[[count(*)]]
+ TableScan: person
+ "#
+ );
+ assert_placeholder_type(&plan, "$1", DataType::Int32);
+}
+
+/// The same, for a grouping expression repeated in HAVING.
+#[test]
+fn select_aggregate_with_having_placeholder_expression() {
+ let sql = "SELECT CASE WHEN age < $1 THEN 'young' ELSE 'old' END, count(*)
+ FROM person
+ GROUP BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END
+ HAVING CASE WHEN age < $1 THEN 'young' ELSE 'old' END = 'young'";
+ let plan = logical_plan(sql).unwrap();
+ assert_snapshot!(
+ plan,
+ @r#"
+ Projection: CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END, count(*)
+ Filter: CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END = Utf8("young")
+ Aggregate: groupBy=[[CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END]], aggr=[[count(*)]]
+ TableScan: person
+ "#
+ );
+ assert_placeholder_type(&plan, "$1", DataType::Int32);
+}
+
+/// The same, for a grouping expression repeated in ORDER BY.
+#[test]
+fn select_aggregate_with_order_by_placeholder_expression() {
+ let sql = "SELECT CASE WHEN age < $1 THEN 'young' ELSE 'old' END, count(*)
+ FROM person
+ GROUP BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END
+ ORDER BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END";
+ let plan = logical_plan(sql).unwrap();
+ assert_snapshot!(
+ plan,
+ @r#"
+ Sort: CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END ASC NULLS LAST
+ Projection: CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END, count(*)
+ Aggregate: groupBy=[[CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END]], aggr=[[count(*)]]
+ TableScan: person
+ "#
+ );
+ assert_placeholder_type(&plan, "$1", DataType::Int32);
+}
+
+/// The same, for a window expression repeated in QUALIFY. Here the two spellings
+/// of the window expression collide by name instead, since they print alike but
+/// do not compare equal.
+#[test]
+fn select_window_with_qualify_placeholder_expression() {
+ let sql = "SELECT first_name,
+ row_number() OVER (PARTITION BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END)
+ FROM person
+ QUALIFY row_number() OVER (PARTITION BY CASE WHEN age < $1 THEN 'young' ELSE 'old' END) = 1";
+ let plan = logical_plan(sql).unwrap();
+ assert_snapshot!(
+ plan,
+ @r#"
+ Projection: person.first_name, row_number() PARTITION BY [CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
+ Filter: row_number() PARTITION BY [CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING = Int64(1)
+ WindowAggr: windowExpr=[[row_number() PARTITION BY [CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]]
+ TableScan: person
+ "#
+ );
+ assert_placeholder_type(&plan, "$1", DataType::Int32);
+}
+
+#[test]
+fn select_distinct_on_with_order_by_placeholder_expression() {
+ let 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";
+ let plan = logical_plan(sql).unwrap();
+ assert_snapshot!(
+ plan,
+ @r#"
+ DistinctOn: on_expr=[[CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END]], select_expr=[[person.first_name]], sort_expr=[[CASE WHEN person.age < $1 THEN Utf8("young") ELSE Utf8("old") END ASC NULLS LAST]]
+ TableScan: person
+ "#
+ );
+ assert_placeholder_type(&plan, "$1", DataType::Int32);
+}
+
#[test]
fn select_binary_expr() {
let sql = "SELECT age + salary from person";
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org