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/09/29 01:42:04 UTC
(datafusion) branch main updated: fix: Error if both `group_by` and `aggr` is empty in `AggregateExec::try_new()` (#25786)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new bdad988f23 fix: Error if both `group_by` and `aggr` is empty in `AggregateExec::try_new()` (#25786)
bdad988f23 is described below
commit bdad988f23f4f8a45a71409e7d7f24a326ca909c
Author: Yongting You <20...@gmail.com>
AuthorDate: Tue Sep 29 01:31:20 2026 +0000
fix: Error if both `group_by` and `aggr` is empty in `AggregateExec::try_new()` (#25786)
## 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 #.
## 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.
Please explain the problem you are trying to solve in terms of the
user-visible
behavior, rather than the implementation.
For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of
the
implementation. "COUNT(DISTINCT) returns wrong results when the column
contains
nulls" is the user-visible problem.
-->
If both group by expression and aggregate expression is empty, such
aggregate is not valid.
```
AggregateExec {
group_by_expr: vec![],
aggr_expr: vec![],
....
}
```
Now `AggregateExec::try_new()` with such invalid input would succeed,
this PR adds a validation for it. (this case is not possible from SQL
interface, but it can be built through raw `AggregateExec` API)
## 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.
-->
1. Error for this case
2. Update several optimizer UTs to avoid building such plan
## What is the testing strategy for this PR?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. Serve as another way to document the expected behavior of the code
Briefly describe how this PR is tested, and point to the specific tests
you added. For example: 'This new feature is covered by the
`sqllogictest` cases added in `foo.slt`'.
If this PR does not add tests, explain why. For example, if the change
is already covered by existing tests, please mention it.
You should also check the `codecov` bot reply on this PR to confirm the
changed code is exercised.
-->
UT (this case it not able to test from SQL)
## Are there any user-facing changes?
<!--
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.
-->
no
---
.../tests/physical_optimizer/enforce_sorting.rs | 4 +--
.../limited_distinct_aggregation.rs | 12 +++----
.../core/tests/physical_optimizer/test_utils.rs | 6 ++--
datafusion/physical-plan/src/aggregates/mod.rs | 42 ++++++++++++++++++++--
datafusion/sqllogictest/test_files/grouping.slt | 6 ++++
5 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/datafusion/core/tests/physical_optimizer/enforce_sorting.rs b/datafusion/core/tests/physical_optimizer/enforce_sorting.rs
index 04019feb66..b5a2a720cb 100644
--- a/datafusion/core/tests/physical_optimizer/enforce_sorting.rs
+++ b/datafusion/core/tests/physical_optimizer/enforce_sorting.rs
@@ -2095,7 +2095,7 @@ async fn test_remove_unnecessary_sort3() -> Result<()> {
EnforceSortingTest::new(physical_plan.clone()).with_repartition_sorts(true);
assert_snapshot!(test.run(), @r"
Input Plan:
- AggregateExec: mode=Final, gby=[], aggr=[]
+ AggregateExec: mode=Single, gby=[], aggr=[COUNT(*)]
SortPreservingMergeExec: [nullable_col@0 ASC, non_nullable_col@1 ASC]
SortExec: expr=[nullable_col@0 ASC, non_nullable_col@1 ASC], preserve_partitioning=[true]
RepartitionExec: partitioning=RoundRobinBatch(10), input_partitions=1, maintains_sort_order=true
@@ -2104,7 +2104,7 @@ async fn test_remove_unnecessary_sort3() -> Result<()> {
DataSourceExec: partitions=1, partition_sizes=[0]
Optimized Plan:
- AggregateExec: mode=Final, gby=[], aggr=[]
+ AggregateExec: mode=Single, gby=[], aggr=[COUNT(*)]
DataSourceExec: partitions=1, partition_sizes=[0]
");
diff --git a/datafusion/core/tests/physical_optimizer/limited_distinct_aggregation.rs b/datafusion/core/tests/physical_optimizer/limited_distinct_aggregation.rs
index 9a43ba01da..1d5737d243 100644
--- a/datafusion/core/tests/physical_optimizer/limited_distinct_aggregation.rs
+++ b/datafusion/core/tests/physical_optimizer/limited_distinct_aggregation.rs
@@ -532,14 +532,14 @@ fn test_no_group_by() -> Result<()> {
let source = mock_data()?;
let schema = source.schema();
- // `SELECT <aggregate with no expressions> FROM DataSourceExec LIMIT 10;`, Single AggregateExec
+ // `SELECT COUNT(*) FROM DataSourceExec LIMIT 10;`, Single AggregateExec
let single_agg = AggregateExec::try_new(
AggregateMode::Single,
build_group_by(&schema, vec![]),
- vec![], /* aggr_expr */
- vec![], /* filter_expr */
- source, /* input */
- schema, /* input_schema */
+ vec![Arc::new(TestAggregate::CountStar.count_expr(&schema))],
+ vec![None],
+ source,
+ schema,
)?;
let limit_exec = LocalLimitExec::new(
Arc::new(single_agg),
@@ -553,7 +553,7 @@ fn test_no_group_by() -> Result<()> {
actual,
@r"
LocalLimitExec: fetch=10
- AggregateExec: mode=Single, gby=[], aggr=[]
+ AggregateExec: mode=Single, gby=[], aggr=[COUNT(*)]
DataSourceExec: partitions=1, partition_sizes=[1]
"
);
diff --git a/datafusion/core/tests/physical_optimizer/test_utils.rs b/datafusion/core/tests/physical_optimizer/test_utils.rs
index 6b10efffa9..3733dba39c 100644
--- a/datafusion/core/tests/physical_optimizer/test_utils.rs
+++ b/datafusion/core/tests/physical_optimizer/test_utils.rs
@@ -378,10 +378,10 @@ pub fn aggregate_exec(input: Arc<dyn ExecutionPlan>) -> Arc<dyn ExecutionPlan> {
let schema = input.schema();
Arc::new(
AggregateExec::try_new(
- AggregateMode::Final,
+ AggregateMode::Single,
PhysicalGroupBy::default(),
- vec![],
- vec![],
+ vec![Arc::new(TestAggregate::CountStar.count_expr(&schema))],
+ vec![None],
input,
schema,
)
diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs
index d82d6de61c..9437f1d83e 100644
--- a/datafusion/physical-plan/src/aggregates/mod.rs
+++ b/datafusion/physical-plan/src/aggregates/mod.rs
@@ -979,7 +979,14 @@ impl AggregateExec {
&self.cache
}
- /// Create a new hash aggregate execution plan
+ /// Create a new hash aggregate execution plan.
+ ///
+ /// # Errors
+ ///
+ /// Returns an internal error if the provided args can't build a valid aggregation:
+ ///
+ /// - Both grouping and aggregate expressions are empty, without grouping sets.
+ /// - The numbers of aggregate expressions and filter expressions differ.
pub fn try_new(
mode: AggregateMode,
group_by: impl Into<Arc<PhysicalGroupBy>>,
@@ -1023,6 +1030,12 @@ impl AggregateExec {
let group_by = group_by.into();
let filter_expr = filter_expr.into();
+ if group_by.is_true_no_grouping() && aggr_expr.is_empty() {
+ return internal_err!(
+ "Grouping expressions and aggregate expressions cannot both be empty"
+ );
+ }
+
// Make sure arguments are consistent in size
assert_eq_or_internal_err!(
aggr_expr.len(),
@@ -3350,7 +3363,7 @@ mod tests {
use arrow::compute::{SortOptions, concat_batches};
use arrow::datatypes::{Int32Type, Int64Type};
use datafusion_common::test_util::{batches_to_sort_string, batches_to_string};
- use datafusion_common::{DataFusionError, internal_err};
+ use datafusion_common::{DataFusionError, assert_contains, internal_err};
use datafusion_execution::config::SessionConfig;
use datafusion_execution::memory_pool::{
FairSpillPool, MemoryPool, PeakRecordingPool,
@@ -3414,6 +3427,31 @@ mod tests {
Ok(schema)
}
+ /// If both group_by and aggr is empty, return error since it's not a valid aggregate
+ #[test]
+ fn test_try_new_rejects_empty_aggregate() -> Result<()> {
+ let schema = create_test_schema()?;
+ for group_by in [
+ PhysicalGroupBy::default(),
+ PhysicalGroupBy::new_single(vec![]),
+ ] {
+ let err = AggregateExec::try_new(
+ AggregateMode::Single,
+ group_by,
+ vec![],
+ vec![],
+ Arc::new(EmptyExec::new(Arc::clone(&schema))),
+ Arc::clone(&schema),
+ )
+ .unwrap_err();
+ assert_contains!(
+ err.to_string(),
+ "Grouping expressions and aggregate expressions cannot both be empty"
+ );
+ }
+ Ok(())
+ }
+
/// some mock data to aggregates
fn some_data() -> (Arc<Schema>, Vec<RecordBatch>) {
// define a schema.
diff --git a/datafusion/sqllogictest/test_files/grouping.slt b/datafusion/sqllogictest/test_files/grouping.slt
index 5e364eb3ce..738b7ae722 100644
--- a/datafusion/sqllogictest/test_files/grouping.slt
+++ b/datafusion/sqllogictest/test_files/grouping.slt
@@ -310,6 +310,12 @@ SELECT COUNT(*) FROM test GROUP BY GROUPING SETS (());
----
2
+# An empty grouping set is valid even without aggregate expressions.
+query I
+SELECT 1 FROM test GROUP BY GROUPING SETS (());
+----
+1
+
# grouping_sets_with_empty_set
query I
SELECT SUM(v1) FROM generate_series(10) AS t1(v1) GROUP BY GROUPING SETS(())
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org