Merge pull request #60615 from max-sixty/prql-panic

fix(prql): Robust panic handler

.gitignore

@@ -165,7 +165,7 @@ tests/queries/0_stateless/*.expect.history
 tests/integration/**/_gen
 
 # rust
-/rust/**/target
+/rust/**/target*
 # It is autogenerated from *.in
 /rust/**/.cargo/config.toml
 /rust/**/vendor

rust/Cargo.lock

@@ -6,6 +6,7 @@ version = 3
 name = "_ch_rust_prql"
 version = "0.1.0"
 dependencies = [
+ "anstream",
  "prqlc",
  "serde_json",
 ]
@@ -698,9 +699,9 @@ checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a"
 
 [[package]]
 name = "miniz_oxide"
-version = "0.7.1"
+version = "0.7.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "e7810e0be55b428ada41041c41f32c9f1a42817901b4ccf45fa3d4b6561e74c7"
+checksum = "9d811f3e15f28568be3407c8e7fdb6514c1cda3cb30683f15b6a1a1dc4ea14a7"
 dependencies = [
  "adler",
 ]
@@ -751,9 +752,9 @@ dependencies = [
 
 [[package]]
 name = "object"
-version = "0.32.1"
+version = "0.32.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9cf5f9dd3933bd50a9e1f149ec995f39ae2c496d31fd772c1fd45ebc27e902b0"
+checksum = "a6a622008b6e321afc04970976f62ee297fdbaa6f95318ca343e3eebb9648441"
 dependencies = [
  "memchr",
 ]

rust/prql/Cargo.toml

@@ -4,6 +4,7 @@ name = "_ch_rust_prql"
 version = "0.1.0"
 
 [dependencies]
+anstream = {version = "0.6.12"}
 prqlc = {version = "0.11.3", default-features = false}
 serde_json = "1.0"
 

rust/prql/src/lib.rs

@@ -39,6 +39,11 @@ pub unsafe extern "C" fn prql_to_sql_impl(
     };
 
     if let Ok(sql_str) = prqlc::compile(&query_str, &opts) {
+        // NOTE: Over at PRQL we're considering to un-deprecate & re-enable the
+        // `color: false` option. If that happens, we can remove the `strip_str`
+        // here, which strips color codes from the output.
+        use anstream::adapter::strip_str;
+        let sql_str = strip_str(&sql_str).to_string();
         set_output(sql_str, out, out_size);
         0
     } else {
@@ -54,17 +59,50 @@ pub unsafe extern "C" fn prql_to_sql(
     out: *mut *mut u8,
     out_size: *mut u64,
 ) -> i64 {
-    let ret = panic::catch_unwind(|| {
-        return prql_to_sql_impl(query, size, out, out_size);
-    });
-    return match ret {
     // NOTE: using cxxbridge we can return proper Result<> type.
-        Err(_err) => 1,
-        Ok(res) => res,
-    }
+    panic::catch_unwind(|| prql_to_sql_impl(query, size, out, out_size)).unwrap_or_else(|_| {
+        set_output("prqlc panicked".to_string(), out, out_size);
+        1
+    })
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn prql_free_pointer(ptr_to_free: *mut u8) {
     std::mem::drop(CString::from_raw(ptr_to_free as *mut c_char));
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::ffi::{CStr, CString};
+
+    /// A test helper to offer a rust interface to the C bindings
+    fn run_compile(query: &str) -> (String, i64) {
+        let query_cstr = CString::new(query).unwrap();
+        let query_ptr = query_cstr.as_ptr() as *const u8;
+        let query_size = query_cstr.to_bytes_with_nul().len() as u64 - 1; // Excluding the null terminator
+
+        let mut out: *mut u8 = std::ptr::null_mut();
+        let mut out_size = 0_u64;
+
+        unsafe {
+            let success = prql_to_sql(query_ptr, query_size, &mut out, &mut out_size);
+            let output = CStr::from_ptr(out as *const i8)
+                .to_str()
+                .unwrap()
+                .to_string();
+            prql_free_pointer(out);
+            (output, success)
+        }
+    }
+
+    #[test]
+    fn test_prql_to_sql() {
+        assert!(run_compile("from x").0.contains("SELECT"));
+        assert!(run_compile("asdf").1 == 1);
+        // In prqlc 0.11.3, this is a panic, so that allows us to test that the
+        // panic is caught. When we upgrade prqlc, it won't be a panic any
+        // longer.
+        assert!(run_compile("x -> y").1 == 1);
+    }
+}

tests/queries/0_stateless/03003_prql_panic.reference

@@ -0,0 +1 @@
+SYNTAX_ERROR

tests/queries/0_stateless/03003_prql_panic.sh

@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+# Tags: no-fasttest
+# Requires Rust, which is not built for Fast Test.
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+# Before [1] this causes a panic, but it will be fixed soon, so do not check
+# for panic, but just for SYNTAX_ERROR.
+#
+#   [1]: https://github.com/PRQL/prql/pull/4285
+$CLICKHOUSE_CLIENT --dialect prql -q "SELECT id FROM distributed_test_table GROUP BY x -> concat(concat(materialize(toNullable(NULL)))) LIMIT 3" |& grep -o -m1 SYNTAX_ERROR