Allow dialects that use -> as an operator to support LAMBDA syntax - #2458
Allow dialects that use -> as an operator to support LAMBDA syntax#2458adriangb wants to merge 5 commits into
-> as an operator to support LAMBDA syntax#2458Conversation
| fn supports_lambda_keyword_syntax(&self) -> bool { | ||
| self.supports_lambda_functions() | ||
| } |
There was a problem hiding this comment.
One could argue for adding fn supports_lambda_arrow_syntax() as well, but I'd hold off until there is a concrete use case for enabling arrow syntax but not lambda syntax.
There was a problem hiding this comment.
Arrow-only is already expressible by overriding supports_lambda_keyword_syntax to false, so I believe there is no need.
| #[test] | ||
| fn test_lambda_keyword_syntax_on_postgres_derivative() { |
There was a problem hiding this comment.
I'm split between this (the real regression test I want) and another test using a MyDialect in sqlparser_custom_dialect.rs that enables the two flags. Open to input.
There was a problem hiding this comment.
Both seem worth keeping.
|
@LucaCappelletti94 could I ask you to take a look at this change? Thanks! |
LucaCappelletti94
left a comment
There was a problem hiding this comment.
Generally ok, just a missing Snowflake syntax test, and the missing documentation to lambda, and the wrong syntax in the example.
| panic!("expected `->` to stay a binary operator"); | ||
| }; | ||
| assert_eq!(&BinaryOperator::Arrow, op); | ||
| } |
There was a problem hiding this comment.
I believe we should add a test for the Spark/Snowflake case (they only accept ->):
| } | |
| } | |
| #[test] | |
| fn custom_dialect_lambda_arrow_syntax_without_keyword() { | |
| // Arrow lambdas stay on while the `LAMBDA` keyword spelling is off, | |
| // as in engines like Spark and Snowflake. | |
| #[derive(Debug)] | |
| struct MyDialect {} | |
| impl Dialect for MyDialect { | |
| fn is_identifier_start(&self, ch: char) -> bool { | |
| is_identifier_start(ch) | |
| } | |
| fn is_identifier_part(&self, ch: char) -> bool { | |
| is_identifier_part(ch) | |
| } | |
| fn supports_lambda_functions(&self) -> bool { | |
| true | |
| } | |
| fn supports_lambda_keyword_syntax(&self) -> bool { | |
| false | |
| } | |
| } | |
| let dialect = MyDialect {}; | |
| let sql = "SELECT transform(xs, x -> x + 1)"; | |
| assert_eq!( | |
| sql, | |
| &format!("{}", Parser::parse_sql(&dialect, sql).unwrap()[0]) | |
| ); | |
| assert!(Parser::parse_sql(&dialect, "SELECT transform(xs, lambda x : x + 1)").is_err()); | |
| } |
There was a problem hiding this comment.
Thanks, added but in sqlparser_custom_dialect.rs
| /// lambda functions, for example: | ||
| /// | ||
| /// ```sql | ||
| /// SELECT transform(array(1, 2, 3), LAMBDA x : x + 1); -- returns [2,3,4] |
There was a problem hiding this comment.
I believe transform(array(...), LAMBDA ...) is the wrong spelling, if you meant the DuckDB syntax it would be more like:
| /// SELECT transform(array(1, 2, 3), LAMBDA x : x + 1); -- returns [2,3,4] | |
| /// SELECT list_transform([1, 2, 3], lambda x : x + 1); -- returns [2, 3, 4] |
| /// and its derivatives, where `->` is JSON member access. Defaults to | ||
| /// [`Self::supports_lambda_functions`], so dialects supporting the `->` | ||
| /// spelling accept the `LAMBDA` spelling too unless they say otherwise. | ||
| fn supports_lambda_keyword_syntax(&self) -> bool { |
There was a problem hiding this comment.
| fn supports_lambda_keyword_syntax(&self) -> bool { | |
| /// | |
| /// See <https://duckdb.org/docs/stable/sql/functions/lambda> | |
| fn supports_lambda_keyword_syntax(&self) -> bool { |
| fn supports_lambda_keyword_syntax(&self) -> bool { | ||
| self.supports_lambda_functions() | ||
| } |
There was a problem hiding this comment.
Arrow-only is already expressible by overriding supports_lambda_keyword_syntax to false, so I believe there is no need.
| #[test] | ||
| fn test_lambda_keyword_syntax_on_postgres_derivative() { |
There was a problem hiding this comment.
Both seem worth keeping.
2d11f84 to
d994419
Compare
Exercises the capability the way a downstream crate would: derive a dialect from PostgreSqlDialect with `supports_lambda_keyword_syntax` overridden, then check that `LAMBDA x : x + 1` parses while `->` and `->>` keep parsing as JSON member access rather than lambda parameters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Address review feedback: rather than parsing the `LAMBDA` spelling and the `->` operator as separate statements, parse one expression that uses both — a lambda whose body is a JSON access — which is the shape a PostgreSQL derivative actually cares about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three changes are applied review suggestions from Luca Cappelletti. Co-Authored-By: Luca Cappelletti <7738570+LucaCappelletti94@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d994419 to
1d89c66
Compare
|
Thanks for the review @LucaCappelletti94! @iffyio or @yoavcloud could one of you take a look at this PR that @LucaCappelletti94 has already reviewed and approved? Thanks! |
iffyio
left a comment
There was a problem hiding this comment.
left a comment, its not clear to me what the PR is looking to solve for
| /// [`Self::supports_lambda_functions`], so dialects supporting the `->` | ||
| /// spelling accept the `LAMBDA` spelling too unless they say otherwise. | ||
| /// | ||
| /// See <https://duckdb.org/docs/stable/sql/functions/lambda> |
There was a problem hiding this comment.
Im not sure I understood the problem being solved for - the description mentions pg as an example, suggesting there is some ambiguous grammar in play but pg doesnt have lambda syntax to my knowledge?
is there an example syntax that issupported by a dialect and the parser doesnt cover?
|
you cannot currently have a grammar that supports both postgres style json operators and lambda transforms because |
Motivation
supports_lambda_functions()gates two different spellings of the same feature:x -> x + 1LAMBDAkeyword form,LAMBDA x : x + 1Because they share one flag, a dialect cannot have one without the other. That
shuts out any dialect that already gives
->a meaning. PostgreSQL is theobvious case:
->is JSON member access, so turning the flag on silentlyreinterprets existing expressions rather than adding a capability.
Concretely, with the flag enabled,
a -> 'b'no longer parses as a binaryoperator. It parses as a lambda with parameter
aand body'b', becauseparse_prefixtreats any unreserved word followed by->as a lambdaparameter. Note
->>is unaffected, so the breakage is partial and easy tomiss.
The
LAMBDAkeyword form has no such conflict: it does not claim->.Change
Adds
Dialect::supports_lambda_keyword_syntax(), which gates only theLAMBDAkeyword form, and defaults to
supports_lambda_functions().No existing dialect changes behavior. Dialects that support the arrow form keep
both spellings; dialects that support neither still get neither. A dialect that
uses
->for something else can now override just the new method to get lambdaswithout disturbing its operator.
No dialect shipped here opts in; this only makes the capability reachable.
Tests
Two tests in
tests/sqlparser_custom_dialect.rs:supports_lambda_keyword_syntaxparseslambda x : x + 1while->stays aBinaryOperator::Arrowsupports_lambda_functionsstill accepts bothspellings, pinning the defaulting behavior
Full suite,
cargo fmt --check, andcargo clippy --all-targets --all-featuresall pass.