From 0595f54c994ea875b80d2fadc6cd830f7a78b5d0 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Thu, 3 Jun 2021 21:21:31 +0200 Subject: [PATCH 1/8] Fix aggregate fn with invalid column --- datafusion/src/physical_plan/aggregates.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/datafusion/src/physical_plan/aggregates.rs b/datafusion/src/physical_plan/aggregates.rs index 3607f29debba..c0c3fc9e3401 100644 --- a/datafusion/src/physical_plan/aggregates.rs +++ b/datafusion/src/physical_plan/aggregates.rs @@ -113,7 +113,14 @@ pub fn create_aggregate_expr( name: String, ) -> Result> { // coerce - let arg = coerce(args, input_schema, &signature(fun))?[0].clone(); + let arg = coerce(args, input_schema, &signature(fun))?; + if arg.len() == 0 { + return Err(DataFusionError::Plan(format!( + "Invalid aggregation '{}'", + name, + ))); + } + let arg = arg[0].clone(); let arg_types = args .iter() From 0bf891d92a3bfdfe3af1dd138d990051cc567b8d Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Fri, 4 Jun 2021 21:03:46 +0200 Subject: [PATCH 2/8] Fix error message --- datafusion/src/physical_plan/aggregates.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/src/physical_plan/aggregates.rs b/datafusion/src/physical_plan/aggregates.rs index c0c3fc9e3401..4bae251fec43 100644 --- a/datafusion/src/physical_plan/aggregates.rs +++ b/datafusion/src/physical_plan/aggregates.rs @@ -116,7 +116,7 @@ pub fn create_aggregate_expr( let arg = coerce(args, input_schema, &signature(fun))?; if arg.len() == 0 { return Err(DataFusionError::Plan(format!( - "Invalid aggregation '{}'", + "Aggregate error: Invalid or wrong number of arguments passed to aggregate: '{}'", name, ))); } From c1078acfca09209d446d7d558071640bd5cf9613 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Fri, 4 Jun 2021 21:09:16 +0200 Subject: [PATCH 3/8] Fix error message --- datafusion/src/physical_plan/aggregates.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/src/physical_plan/aggregates.rs b/datafusion/src/physical_plan/aggregates.rs index 4bae251fec43..a5ffea038487 100644 --- a/datafusion/src/physical_plan/aggregates.rs +++ b/datafusion/src/physical_plan/aggregates.rs @@ -116,7 +116,7 @@ pub fn create_aggregate_expr( let arg = coerce(args, input_schema, &signature(fun))?; if arg.len() == 0 { return Err(DataFusionError::Plan(format!( - "Aggregate error: Invalid or wrong number of arguments passed to aggregate: '{}'", + "Aggregate error. Invalid or wrong number of arguments passed to aggregate: '{}'", name, ))); } From 5b50308e5af019eb5ea90e296712d2a41f1e4b7e Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Fri, 4 Jun 2021 21:30:17 +0200 Subject: [PATCH 4/8] fix clippy --- datafusion/src/physical_plan/aggregates.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/src/physical_plan/aggregates.rs b/datafusion/src/physical_plan/aggregates.rs index a5ffea038487..c0aa7899592d 100644 --- a/datafusion/src/physical_plan/aggregates.rs +++ b/datafusion/src/physical_plan/aggregates.rs @@ -114,7 +114,7 @@ pub fn create_aggregate_expr( ) -> Result> { // coerce let arg = coerce(args, input_schema, &signature(fun))?; - if arg.len() == 0 { + if arg.is_empty() { return Err(DataFusionError::Plan(format!( "Aggregate error. Invalid or wrong number of arguments passed to aggregate: '{}'", name, From 80701f93b016df43bdc30fed854da39bfea1f384 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Fri, 4 Jun 2021 23:39:11 +0200 Subject: [PATCH 5/8] fix message and test --- datafusion/src/physical_plan/aggregates.rs | 2 +- datafusion/tests/sql.rs | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/datafusion/src/physical_plan/aggregates.rs b/datafusion/src/physical_plan/aggregates.rs index c0aa7899592d..60025a316228 100644 --- a/datafusion/src/physical_plan/aggregates.rs +++ b/datafusion/src/physical_plan/aggregates.rs @@ -116,7 +116,7 @@ pub fn create_aggregate_expr( let arg = coerce(args, input_schema, &signature(fun))?; if arg.is_empty() { return Err(DataFusionError::Plan(format!( - "Aggregate error. Invalid or wrong number of arguments passed to aggregate: '{}'", + "Invalid or wrong number of arguments passed to aggregate: '{}'", name, ))); } diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs index 029e9307e5f6..163ff892524b 100644 --- a/datafusion/tests/sql.rs +++ b/datafusion/tests/sql.rs @@ -3437,3 +3437,14 @@ async fn test_physical_plan_display_indent_multi_children() { expected, actual ); } + +#[tokio::test] +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).unwrap(); + let physical_plan = ctx.create_physical_plan(&logical_plan); + assert!(physical_plan.is_err()); + Ok(()) +} From 9db2e5f8b700a63324b32073c9cbc62e8634eff1 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Fri, 4 Jun 2021 23:43:08 +0200 Subject: [PATCH 6/8] avoid unwrap in test_aggregation_with_bad_arguments --- datafusion/tests/sql.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs index 163ff892524b..02b1c6ca746b 100644 --- a/datafusion/tests/sql.rs +++ b/datafusion/tests/sql.rs @@ -3443,7 +3443,7 @@ 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).unwrap(); + let logical_plan = ctx.create_logical_plan(&sql)?; let physical_plan = ctx.create_physical_plan(&logical_plan); assert!(physical_plan.is_err()); Ok(()) From bd5e062e14c9bf01a07aca0dfed4d9ef7eeffff2 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Sun, 6 Jun 2021 12:39:20 +0200 Subject: [PATCH 7/8] Update datafusion/tests/sql.rs Co-authored-by: Andrew Lamb --- datafusion/tests/sql.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs index 02b1c6ca746b..7368dc866d03 100644 --- a/datafusion/tests/sql.rs +++ b/datafusion/tests/sql.rs @@ -3445,6 +3445,7 @@ async fn test_aggregation_with_bad_arguments() -> Result<()> { 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()); + let err = physical_plan.unwrap_err(); + assert_eq!(err.to_string(), "Invalid or wrong number of arguments passed to aggregate: 'COUNT'"); Ok(()) } From a8f801a630a53953dd5d435f564488e2cd2644d8 Mon Sep 17 00:00:00 2001 From: Javier Goday Date: Sun, 6 Jun 2021 12:49:29 +0200 Subject: [PATCH 8/8] Fix test_aggregation_with_bad_arguments --- datafusion/tests/sql.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/tests/sql.rs b/datafusion/tests/sql.rs index 7368dc866d03..f4b26791ee64 100644 --- a/datafusion/tests/sql.rs +++ b/datafusion/tests/sql.rs @@ -3446,6 +3446,6 @@ async fn test_aggregation_with_bad_arguments() -> Result<()> { let logical_plan = ctx.create_logical_plan(&sql)?; let physical_plan = ctx.create_physical_plan(&logical_plan); let err = physical_plan.unwrap_err(); - assert_eq!(err.to_string(), "Invalid or wrong number of arguments passed to aggregate: 'COUNT'"); + assert_eq!(err.to_string(), "Error during planning: Invalid or wrong number of arguments passed to aggregate: 'COUNT(DISTINCT )'"); Ok(()) }