feat(snql) Support higher order functions in SnQL #2333
Conversation
SnQL now supports using lambdas in its clauses. Lambdas are defined as a tuple of identifiers, followed by an arrow, followed by a function that may or may not use those identifiers. An identifier in SnQL is a set of characters enclosed in backticks. The distinction to use backticks was to make parsing and validating easier, since the parser doesn't have to try and determine if a string is a column or an identifier. This feature set is slightly more limited than what is available in Clickhouse. The major difference is that a lambda can only be used as a parameter in a function, and the transformation of a lambda must always be a function itself. So (`x`) -> `x` is not a valid lambda. This change makes using higher order functions such as arrayMap possible, since those functions required a lambda to be defined to use them.
Codecov Report
@@ Coverage Diff @@
## master #2333 +/- ##
==========================================
+ Coverage 92.76% 92.80% +0.03%
==========================================
Files 561 561
Lines 25790 25834 +44
==========================================
+ Hits 23924 23974 +50
+ Misses 1866 1860 -6
Continue to review full report at Codecov.
|
|
Just an initial comment from looking at the PR description. This would be much clearer with an example:
This as well:
It's not totally clear what is a valid lambda where. To make it more clear you could say: The following is valid in clickhouse sql: It is not valid in snql because a lambda can only be used as a parameter in a |
|
@vladkluev I added examples, hope that makes it clearer. |
| @@ -1119,6 +1161,33 @@ def mangle_column_value(exp: Expression) -> Expression: | |||
| query.transform_expressions(mangle_column_value) | |||
|
|
|||
|
|
|||
| def validate_identifiers_in_lambda( | |||
There was a problem hiding this comment.
one thing I remembered before you merge. You don' test the recursive case of this function, you just have the one test. You may want to do that
There was a problem hiding this comment.
Thanks for the suggestion, I added a new test and tweaked the code as a result.
…m:getsentry/snuba into evanh/feat/support-higher-order-functions
SnQL now supports using lambdas in its clauses. Lambdas are defined as a tuple
of identifiers, followed by an arrow, followed by a function that may or may not
use those identifiers.
An identifier in SnQL is a set of characters enclosed in
backticks. The distinction to use backticks was to make parsing and validating
easier, since the parser doesn't have to try and determine if a string is a
column or an identifier.
This feature set is slightly more limited than what is available in Clickhouse.
The major difference is that a lambda can only be used as a parameter in a
function, and the transformation of a lambda must always be a function itself.
This change makes using higher order functions such as arrayMap possible, since
those functions require a lambda to be defined to use them.