Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong aggregation arguments error. #505

Merged
merged 8 commits into from
Jun 8, 2021
Merged

Wrong aggregation arguments error. #505

merged 8 commits into from
Jun 8, 2021

Conversation

jgoday
Copy link
Contributor

@jgoday jgoday commented Jun 4, 2021

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
Copy link

codecov-commenter commented Jun 4, 2021

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 Files Coverage Δ
datafusion/src/physical_plan/aggregates.rs 91.35% <100.00%> (+0.33%) ⬆️
datafusion/tests/sql.rs 99.28% <100.00%> (+<0.01%) ⬆️
datafusion/src/optimizer/utils.rs 48.22% <0.00%> (-1.78%) ⬇️
...ta/rust/core/src/serde/physical_plan/from_proto.rs 38.65% <0.00%> (-0.99%) ⬇️
datafusion/src/physical_plan/hash_join.rs 85.52% <0.00%> (-0.93%) ⬇️
...sta/rust/core/src/serde/logical_plan/from_proto.rs 35.91% <0.00%> (-0.26%) ⬇️
...ista/rust/core/src/serde/physical_plan/to_proto.rs 50.31% <0.00%> (-0.16%) ⬇️
datafusion/src/physical_plan/planner.rs 80.19% <0.00%> (-0.14%) ⬇️
datafusion/src/logical_plan/builder.rs 90.04% <0.00%> (-0.05%) ⬇️
python/src/dataframe.rs 0.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.

Copy link
Contributor

@alamb alamb left a comment

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?

@alamb alamb added the datafusion Changes in the datafusion crate label Jun 4, 2021
@jgoday
Copy link
Contributor Author

jgoday commented Jun 4, 2021

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

async fn test_aggregation_with_bad_arguments() -> Result<()> {
    let mut 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(())
}

Copy link
Contributor

@alamb alamb left a comment

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

datafusion/tests/sql.rs Outdated Show resolved Hide resolved
jgoday and others added 2 commits June 6, 2021 12:39
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Copy link
Contributor

@alamb alamb left a comment

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:master Jun 8, 2021
@houqp houqp added the bug Something isn't working label Jul 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datafusion Changes in the datafusion crate
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