fix: Add support for expressions on left hand side of std.in operator #4498
Conversation
max-sixty
changed the title
|
@kgutwin very nice fix! Merging now — lmk if you want a release soon, was planning to do one in a few days, can push up if needed. Great if you can add a changelog in another PR, no stress tho |
|
(+ if you can run |
| @@ -1155,6 +1155,8 @@ fn test_in_values_01() { | |||
| from employees | |||
| filter (title | in ["Sales Manager", "Sales Support Agent"]) | |||
| filter (employee_id | in [1, 2, 5]) | |||
| filter (f"{emp_group}.{role}" | in ["sales_ne.mgr", "sales_mw.mgr"]) | |||
| filter (s"{metadata} ->> '$.location'" | in ["Northeast", "Midwest"]) | |||
There was a problem hiding this comment.
| filter (s"{metadata} ->> '$.location'" | in ["Northeast", "Midwest"]) | |
| filter (s"{metadata} -> '$.location'" | in ["Northeast", "Midwest"]) |
?
There was a problem hiding this comment.
Ah, good point.
I thought the tests had passed, but actually the cargo fmt test failed before these had a chance to run. So when we fix fmt, we'll get to test this properly.
It will need a run of task test-rust to update the snapshots (or cargo insta test --accept --features=default,test-dbs)
There was a problem hiding this comment.
@PrettyWood I think ->> is the right operator here, since in ["", ...] is comparing to a VARCHAR which is what ->> returns. Reference: https://duckdb.org/docs/extensions/json.html#json-extraction-functions
There was a problem hiding this comment.
Don't need to delay merging, but FYI adding this to a query in this path https://github.com/PRQL/prql/blob/76c4f767c4312976dba0fbadfa6c27b84e31faa9/prqlc/prqlc/tests/integration/queries/cast.prql would test on duckdb & postgres DBs in CI
| @@ -173,7 +178,10 @@ fn process_array_in(args: &[Expr], ctx: &mut Context) -> Result<sql_ast::Expr> { | |||
| .collect::<Result<Vec<sql_ast::Expr>>>()?, | |||
| negated: false, | |||
| }), | |||
| _ => panic!("args to `std.array_in` must be a column ref and an array"), | |||
| _ => Err( | |||
| Error::new_simple("args to `std.array_in` must be an expression and an array") | |||
There was a problem hiding this comment.
| Error::new_simple("args to `std.array_in` must be an expression and an array") | |
| Error::new_simple("args to `std.array_in` must be an expression and an array respectively") |
|
Thanks @kgutwin ! |
Resolves #4497.
I'm hoping this is as simple as it needs to be to resolve this... if there's a more complex reason why
std.inneeds to not support expressions on the left-hand side, more discussion is appreciated!Thanks for this package - it's making a big impact on our project, which we hope to release relatively soon!