-
Notifications
You must be signed in to change notification settings - Fork 248
Add support for null literals in query parameters #3
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,7 +270,15 @@ pub fn json_params_to_param_map( | |
| let mut map = ParamMap::new(); | ||
| let object = match params { | ||
| Some(Value::Object(object)) => object, | ||
| Some(Value::Null) | None => return Ok(map), | ||
| Some(Value::Null) | None => { | ||
| // Still fill in Literal::Null for declared nullable params. | ||
| for param in query_params { | ||
| if param.nullable { | ||
| map.insert(param.name.clone(), Literal::Null); | ||
| } | ||
| } | ||
| return Ok(map); | ||
| } | ||
| Some(other) => { | ||
| let message = match mode { | ||
| JsonParamMode::Standard => "params must be a JSON object".to_string(), | ||
|
|
@@ -284,12 +292,31 @@ pub fn json_params_to_param_map( | |
|
|
||
| for (key, value) in object { | ||
| let decl = query_params.iter().find(|param| param.name == *key); | ||
| let literal = if let Some(decl) = decl { | ||
| json_value_to_literal_typed(key, value, &decl.type_name, mode)? | ||
| if let Some(decl) = decl { | ||
| if matches!(value, Value::Null) { | ||
| if decl.nullable { | ||
| map.insert(key.clone(), Literal::Null); | ||
| } else { | ||
| return Err(RunInputError::message(format!( | ||
| "param '{}': null is not accepted for non-nullable parameter", | ||
| key | ||
| ))); | ||
| } | ||
| } else { | ||
| let literal = json_value_to_literal_typed(key, value, &decl.type_name, mode)?; | ||
| map.insert(key.clone(), literal); | ||
| } | ||
| } else { | ||
| json_value_to_literal_inferred(key, value, mode)? | ||
| let literal = json_value_to_literal_inferred(key, value, mode)?; | ||
| map.insert(key.clone(), literal); | ||
| }; | ||
| map.insert(key.clone(), literal); | ||
| } | ||
|
|
||
| // Fill in Literal::Null for declared nullable params that were omitted. | ||
| for param in query_params { | ||
| if param.nullable && !map.contains_key(¶m.name) { | ||
| map.insert(param.name.clone(), Literal::Null); | ||
|
Comment on lines
+315
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The new nullable-defaulting logic only runs after parsing a JSON object, so requests that pass Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
|
|
||
| Ok(map) | ||
|
|
@@ -568,15 +595,7 @@ fn json_value_to_literal_inferred( | |
| } | ||
| Ok(Literal::List(out)) | ||
| } | ||
| Value::Null => Err(match mode { | ||
| JsonParamMode::Standard => { | ||
| RunInputError::message(format!("param '{}': null is not supported", key)) | ||
| } | ||
| JsonParamMode::JavaScript => RunInputError::message(format!( | ||
| "param '{}': null values are not supported as query parameters", | ||
| key | ||
| )), | ||
| }), | ||
| Value::Null => Ok(Literal::Null), | ||
| Value::Object(_) => Err(match mode { | ||
| JsonParamMode::Standard => { | ||
| RunInputError::message(format!("param '{}': object is not supported", key)) | ||
|
|
@@ -889,4 +908,110 @@ query q($tags: [String], $days: [Date]?, $due_at: DateTime) { | |
| other => panic!("expected date list param, got {:?}", other), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn nullable_param_omitted_becomes_null() { | ||
| let query = find_named_query( | ||
| "query q($name: String, $bio: String?) { match { $u: User } return { $u } }", | ||
| "q", | ||
| ) | ||
| .expect("query"); | ||
|
|
||
| let params = json_params_to_param_map( | ||
| Some(&json!({ "name": "Alice" })), | ||
| &query.params, | ||
| JsonParamMode::Standard, | ||
| ) | ||
| .expect("should accept omitted nullable param"); | ||
|
|
||
| assert!(matches!(params.get("name"), Some(Literal::String(v)) if v == "Alice")); | ||
| assert!(matches!(params.get("bio"), Some(Literal::Null))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn nullable_param_explicit_null_becomes_null() { | ||
| let query = find_named_query( | ||
| "query q($name: String, $bio: String?) { match { $u: User } return { $u } }", | ||
| "q", | ||
| ) | ||
| .expect("query"); | ||
|
|
||
| let params = json_params_to_param_map( | ||
| Some(&json!({ "name": "Alice", "bio": null })), | ||
| &query.params, | ||
| JsonParamMode::Standard, | ||
| ) | ||
| .expect("should accept explicit null for nullable param"); | ||
|
|
||
| assert!(matches!(params.get("name"), Some(Literal::String(v)) if v == "Alice")); | ||
| assert!(matches!(params.get("bio"), Some(Literal::Null))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn non_nullable_param_rejects_null() { | ||
| let query = find_named_query( | ||
| "query q($name: String) { match { $u: User } return { $u } }", | ||
| "q", | ||
| ) | ||
| .expect("query"); | ||
|
|
||
| let error = json_params_to_param_map( | ||
| Some(&json!({ "name": null })), | ||
| &query.params, | ||
| JsonParamMode::Standard, | ||
| ) | ||
| .expect_err("null for non-nullable param should fail"); | ||
|
|
||
| assert!( | ||
| error | ||
| .to_string() | ||
| .contains("null is not accepted for non-nullable parameter"), | ||
| "unexpected error: {}", | ||
| error | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn nullable_param_with_value_works_normally() { | ||
| let query = find_named_query( | ||
| "query q($bio: String?) { match { $u: User } return { $u } }", | ||
| "q", | ||
| ) | ||
| .expect("query"); | ||
|
|
||
| let params = json_params_to_param_map( | ||
| Some(&json!({ "bio": "hello" })), | ||
| &query.params, | ||
| JsonParamMode::Standard, | ||
| ) | ||
| .expect("should accept string value for nullable param"); | ||
|
|
||
| assert!(matches!(params.get("bio"), Some(Literal::String(v)) if v == "hello")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn inferred_null_param_becomes_literal_null() { | ||
| let params = json_params_to_param_map( | ||
| Some(&json!({ "extra": null })), | ||
| &[], | ||
| JsonParamMode::Standard, | ||
| ) | ||
| .expect("inferred null should succeed"); | ||
|
|
||
| assert!(matches!(params.get("extra"), Some(Literal::Null))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn nullable_params_filled_when_params_is_none() { | ||
| let query = find_named_query( | ||
| "query q($bio: String?) { match { $u: User } return { $u } }", | ||
| "q", | ||
| ) | ||
| .expect("query"); | ||
|
|
||
| let params = json_params_to_param_map(None, &query.params, JsonParamMode::Standard) | ||
| .expect("None params should succeed with nullable declarations"); | ||
|
|
||
| assert!(matches!(params.get("bio"), Some(Literal::Null))); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,7 @@ fn evaluate_expr(batch: &RecordBatch, expr: &IRExpr, params: &ParamMap) -> Resul | |
| /// Create a constant array from a literal value. | ||
| fn literal_to_array(lit: &Literal, num_rows: usize) -> Result<ArrayRef> { | ||
| Ok(match lit { | ||
| Literal::Null => arrow_array::new_null_array(&DataType::Utf8, num_rows), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null literal array hardcodes Utf8 ignoring actual typeMedium Severity
Reviewed by Cursor Bugbot for commit c943d97. Configure here. |
||
| Literal::String(s) => Arc::new(StringArray::from(vec![s.as_str(); num_rows])) as ArrayRef, | ||
| Literal::Integer(n) => { | ||
| // Try to match the most common integer types | ||
|
|
@@ -283,6 +284,7 @@ fn list_scalar_type(items: &[Literal]) -> Result<ScalarType> { | |
|
|
||
| fn literal_scalar_type(lit: &Literal) -> Result<ScalarType> { | ||
| match lit { | ||
| Literal::Null => Ok(ScalarType::String), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null in literal arrays breaks non-String type listsMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit c943d97. Configure here. |
||
| Literal::String(_) => Ok(ScalarType::String), | ||
| Literal::Integer(_) => Ok(ScalarType::I64), | ||
| Literal::Float(_) => Ok(ScalarType::F64), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1225,6 +1225,7 @@ fn ir_expr_to_sql(expr: &IRExpr, params: &ParamMap) -> Option<String> { | |
|
|
||
| pub(super) fn literal_to_sql(lit: &Literal) -> String { | ||
| match lit { | ||
| Literal::Null => "NULL".to_string(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SQL pushdown generates
|
||
| Literal::String(s) => format!("'{}'", s.replace('\'', "''")), | ||
| Literal::Integer(n) => n.to_string(), | ||
| Literal::Float(f) => f.to_string(), | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.