ClickHouse: Support unparenthesized IN right-hand side#2385
ClickHouse: Support unparenthesized IN right-hand side#2385dinmukhamedm wants to merge 3 commits into
Conversation
| #[test] | ||
| fn parse_in_unparenthesized_placeholder() { | ||
| // ClickHouse `{name:Type}` query-parameter placeholder as the IN RHS, without parens. | ||
| match clickhouse().expr_parses_to("x IN {ids:Array(UInt64)}", "x IN ({ids: Array(UInt64)})") { |
There was a problem hiding this comment.
lets rewrite these tests to use all_dialects_where and verified_stmt, we can remove the test for dialects that donts support the feature
There was a problem hiding this comment.
would you like me to move the tests to the common file?
Otherwise, I should have used the function clickhouse() at the bottom of this file that only returns the clickhouse dialect. This will be more consistent with the rest of the tests in this file.
Edit: did both for consistency with the rest of the tests
| Expr::InList { list, negated, .. } => { | ||
| assert!(!negated); | ||
| assert_eq!(list.len(), 1); | ||
| assert!(matches!(list[0], Expr::Dictionary(_))); | ||
| } | ||
| other => panic!("expected InList, got {other:?}"), |
There was a problem hiding this comment.
we can probably drop the ast assertions, I think the funcationality is simple enough to rely only on roundtrip tests
|
|
||
| #[test] | ||
| fn parse_in_unparenthesized_dictionary_placeholder() { | ||
| // The `{name:Type}` placeholder form additionally requires dictionary syntax. | ||
| let dialects = all_dialects_where(|d| { | ||
| d.supports_in_unparenthesized_expr() && d.supports_dictionary_syntax() | ||
| }); | ||
| dialects.expr_parses_to("x IN {ids:Array(UInt64)}", "x IN ({ids: Array(UInt64)})"); | ||
| dialects.expr_parses_to( | ||
| "x NOT IN {ids:Array(UInt64)}", | ||
| "x NOT IN ({ids: Array(UInt64)})", | ||
| ); | ||
| dialects.verified_expr("x IN ({ids: Array(UInt64)})"); | ||
| // Precedence: the trailing `AND` is not swallowed. | ||
| dialects.verified_expr("x IN ({p: Array(UInt64)}) AND y = 1"); | ||
| } |
There was a problem hiding this comment.
is there some special handling for dictionaries we're accounting for? if not I think this test we can probably remove
| // Dialects that accept an unparenthesized IN right-hand side (e.g. ClickHouse) | ||
| // report a different error here, so exclude them. | ||
| let sql = "SELECT POSITION(foo IN) from bar"; | ||
| let res = parse_sql_statements(sql); | ||
| let res = | ||
| all_dialects_except(|d| d.supports_in_unparenthesized_expr()).parse_sql_statements(sql); | ||
| assert_eq!( | ||
| ParserError::ParserError("Expected: (, found: )".to_string()), |
There was a problem hiding this comment.
maybe we can also add a similar assertion for dialects that do support unparenthesized IN?
2 participants
Fixes #2384
Note: The fix is a little broader than just the parameterized ClickHouse queries, e.g.
WHERE id IN {ids: Array(UUID)}.After opening the issue, I experimented a little more with ClickHouse, and found that it wraps in parenthesis anything on the RHS of
IN, even for singular values, e.g.