[arrow-datafusion] branch main updated: fix: Do not panic on invalid placeholders (#5998)

[yjshen]

This is an automated email from the ASF dual-hosted git repository.

yjshen pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-datafusion.git


The following commit(s) were added to refs/heads/main by this push:
     new 03851ce689 fix: Do not panic on invalid placeholders (#5998)
03851ce689 is described below

commit 03851ce6892c4a682058bbc8689dd7d6d7990ad0
Author: Alex Huang <huangweijun1...@gmail.com>
AuthorDate: Sat Apr 15 11:44:41 2023 +0200

    fix: Do not panic on invalid placeholders (#5998)
    
    * fix: enhance error when placeholder is empty
    
    * use DataFusionError::Plan
    
    * update test
    
    * simplify the code
---
 datafusion/expr/src/logical_plan/plan.rs | 40 ++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/datafusion/expr/src/logical_plan/plan.rs 
b/datafusion/expr/src/logical_plan/plan.rs
index e527a0a70c..1e85e13684 100644
--- a/datafusion/expr/src/logical_plan/plan.rs
+++ b/datafusion/expr/src/logical_plan/plan.rs
@@ -605,6 +605,11 @@ impl LogicalPlan {
         expr.transform(&|expr| {
             match &expr {
                 Expr::Placeholder { id, data_type } => {
+                    if id.is_empty() || id == "$0" {
+                        return Err(DataFusionError::Plan(
+                            "Empty placeholder id".to_string(),
+                        ));
+                    }
                     // convert id (in format $1, $2, ..) to idx (0, 1, ..)
                     let idx = id[1..].parse::<usize>().map_err(|e| {
                         DataFusionError::Internal(format!(
@@ -2330,4 +2335,39 @@ mod tests {
         assert_eq!(1, schemas.len());
         assert_eq!(0, schemas[0].fields().len());
     }
+
+    #[test]
+    fn test_replace_invalid_placeholder() {
+        // test empty placeholder
+        let schema = Schema::new(vec![Field::new("id", DataType::Int32, 
false)]);
+
+        let plan = table_scan(TableReference::none(), &schema, None)
+            .unwrap()
+            .filter(col("id").eq(Expr::Placeholder {
+                id: "".into(),
+                data_type: Some(DataType::Int32),
+            }))
+            .unwrap()
+            .build()
+            .unwrap();
+
+        plan.replace_params_with_values(&[42i32.into()])
+            .expect_err("unexpectedly succeeded to replace an invalid 
placeholder");
+
+        // test $0 placeholder
+        let schema = Schema::new(vec![Field::new("id", DataType::Int32, 
false)]);
+
+        let plan = table_scan(TableReference::none(), &schema, None)
+            .unwrap()
+            .filter(col("id").eq(Expr::Placeholder {
+                id: "$0".into(),
+                data_type: Some(DataType::Int32),
+            }))
+            .unwrap()
+            .build()
+            .unwrap();
+
+        plan.replace_params_with_values(&[42i32.into()])
+            .expect_err("unexpectedly succeeded to replace an invalid 
placeholder");
+    }
 }