diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index ea54b83c31..7b53fb44c9 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -1284,28 +1284,10 @@ async fn create_script_internal<'c>( if let Err(e) = windmill_parser::sql_materialize::classify_wrap(&ns.content) { return Err(Error::BadRequest(e.message())); } - // Managed materialize strips line comments when it wraps the SELECT, - // so a `-- $name (TYPE)` declaration is lost while its `$name` - // reference survives in the embedded SELECT — it would run unbound. - // Managed materialize takes no SQL args (the partition is supplied by - // the engine, not bound). Reject declared args with a clear error. - if let Ok(sig) = windmill_parser_sql::parse_duckdb_sig(&ns.content) { - if !sig.args.is_empty() { - let names = sig - .args - .iter() - .map(|a| format!("${}", a.name)) - .collect::>() - .join(", "); - return Err(Error::BadRequest(format!( - "managed `// materialize` cannot take SQL arguments ({names}): wrapping your \ - SELECT drops the `-- $arg` declarations, so they would run unbound. The \ - partition is supplied by the engine — reference its value with the \ - `{{partition}}` token, or use `// materialize manual` to write the DDL (and \ - bind args) yourself." - ))); - } - } + // SQL args are supported: managed materialize strips line comments + // (including `-- $name (type)` declarations) when it wraps the SELECT, + // but the executor parses the signature from the un-wrapped script, so + // `$name` references in the SELECT stay bound at run time. } // `key=` (merge) and `append` are mutually exclusive reconciliation // strategies; append (INSERT-only) wins. Surface the conflict rather diff --git a/backend/windmill-worker/src/duckdb_executor.rs b/backend/windmill-worker/src/duckdb_executor.rs index 3b6a07f702..fc558b9c1c 100644 --- a/backend/windmill-worker/src/duckdb_executor.rs +++ b/backend/windmill-worker/src/duckdb_executor.rs @@ -255,6 +255,14 @@ pub async fn do_duckdb( } else { None }; + // Parse the signature from the ORIGINAL script: managed materialize wraps + // the trailing SELECT and strips line comments, which drops the + // `-- $name (type)` arg declarations while their `$name` references + // survive in the embedded SELECT. Parsing args here (pre-wrap) keeps them + // declared so they are still bound — and s3object args translated to + // `s3://` URIs — at run time. + let sig = parse_duckdb_sig(query)?.args; + let materialized_query; let query: &str = match &materialize { Some((Some(rewritten), _)) => { @@ -263,8 +271,6 @@ pub async fn do_duckdb( } _ => query, }; - - let sig = parse_duckdb_sig(query)?.args; let mut job_args = build_args_values(job, client, conn).await?; let reserved_variables = @@ -1106,6 +1112,40 @@ mod tests { ); } + // Managed `// materialize` may take SQL args (e.g. an s3object uploaded on + // the run form). The wrap strips line comments — including the + // `-- $name (type)` declarations — so the executor parses the signature from + // the original script (done above, before the rewrite) while the `$name` + // references survive inside the wrapped SELECT. This pins both halves of that + // contract so a regression that drops either is caught. + #[test] + fn materialize_preserves_sql_args() { + let script = "-- materialize ducklake://main/rows\n\ + -- $file (s3object)\n\ + SELECT * FROM read_json_auto($file)"; + + // The signature is recoverable from the original (un-wrapped) script. + let sig = parse_duckdb_sig(script).expect("sig parses").args; + let file_arg = sig + .iter() + .find(|a| a.name == "file") + .expect("`$file` declared"); + assert_eq!(file_arg.otyp.as_deref(), Some("s3object")); + + // The wrapped query still references `$file`, so the parsed sig binds it. + let (rewritten, _) = build_materialized_query(script, None) + .expect("materialize builds") + .expect("materialize present"); + let rewritten = rewritten.expect("managed mode rewrites the query"); + assert!( + rewritten.contains("$file"), + "wrapped query must keep the `$file` reference, got:\n{rewritten}" + ); + // The declaration comment is gone (wrap strips line comments) — which is + // exactly why the sig must come from the original, not the rewrite. + assert!(!rewritten.contains("-- $file")); + } + // Tests for parse_attach_db_resource function #[test] fn test_parse_attach_db_resource_postgres_res_prefix() {