Skip to content

Wrong aggregation arguments error. - #505

Merged
alamb merged 8 commits into
apache:masterfrom
jgoday:error_iob
Jun 8, 2021
Merged

Wrong aggregation arguments error.#505
alamb merged 8 commits into
apache:masterfrom
jgoday:error_iob

Conversation

@jgoday

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#496.

Rationale for this change

Check the arguments coerced to an aggregation and return a DataFusionError::Plan if no arguments can be associated with the function call.
Error message will display something like

Aggregate error. Invalid or wrong number of arguments passed to aggregate 'COUNT(DISTINCT )'

What changes are included in this PR?

Checks if there are some valid coerced arguments to call an aggregation function in create_aggregate_expr (datafusion/src/physical_plan/aggregates.rs)

Are there any user-facing changes?

@codecov-commenter

codecov-commenter commented Jun 4, 2021

Copy link
Copy Markdown

Codecov Report

Merging #505 (9db2e5f) into master (28b0dad) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## master #505 +/- ##
==========================================
+ Coverage 75.92% 76.08% +0.16% 
==========================================
Files 154 155 +1 Lines 26195 26555 +360 ==========================================
+ Hits 19889 20205 +316 - Misses 6306 6350 +44 
Impacted FilesCoverage Δ
datafusion/src/physical_plan/aggregates.rs91.35% <100.00%> (+0.33%)⬆️
datafusion/tests/sql.rs99.28% <100.00%> (+<0.01%)⬆️
datafusion/src/optimizer/utils.rs48.22% <0.00%> (-1.78%)⬇️
...ta/rust/core/src/serde/physical_plan/from_proto.rs38.65% <0.00%> (-0.99%)⬇️
datafusion/src/physical_plan/hash_join.rs85.52% <0.00%> (-0.93%)⬇️
...sta/rust/core/src/serde/logical_plan/from_proto.rs35.91% <0.00%> (-0.26%)⬇️
...ista/rust/core/src/serde/physical_plan/to_proto.rs50.31% <0.00%> (-0.16%)⬇️
datafusion/src/physical_plan/planner.rs80.19% <0.00%> (-0.14%)⬇️
datafusion/src/logical_plan/builder.rs90.04% <0.00%> (-0.05%)⬇️
python/src/dataframe.rs0.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28b0dad...9db2e5f. Read the comment docs.

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please also add a test (to sql.rs) that exercises this code path?

@jgoday

Copy link
Copy Markdown
ContributorAuthor

@alamb Added this simple test (trying to create a physical plan with the example that you posted in the issue).

asyncfntest_aggregation_with_bad_arguments() -> Result<()>{letmut ctx = ExecutionContext::new();register_aggregate_csv(&mut ctx)?;let sql = "SELECT COUNT(DISTINCT) FROM aggregate_test_100";let logical_plan = ctx.create_logical_plan(&sql).?;let physical_plan = ctx.create_physical_plan(&logical_plan);assert!(physical_plan.is_err());Ok(())}

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you @jgoday

Comment threaddatafusion/tests/sql.rs Outdated
jgodayand others added 2 commits June 6, 2021 12:39
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jgoday -- this is much nicer than panic! ing :)

@alamb
alamb merged commit 8495f95 into apache:masterJun 8, 2021
@houqphouqp added the bug Something isn't working label Jul 31, 2021
@alambalamb mentioned this pull request Jun 6, 2022
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
* initial commit
* change github workflow
* try to use stable rustfmt
* update CI
* lint
* CI
* clippy
* clippy
* ignore some tests
* remove more nightly references
* add some assertions to make code safe
* revert a change
* use Rust 1.77
* specify rust version 1.77.2
* fix CI
---------
Co-authored-by: Chao Sun <chao@openai.com>
HairstonE pushed a commit to HairstonE/datafusion that referenced this pull request Oct 7, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic! 'index out of bounds: the len is 0 but the index is 0 with bad sql query

4 participants

@jgoday@codecov-commenter@alamb@houqp