Deprecate lambda arrow (->) and replace it with LAMBDA x : x + 1 - #17235

Related issue: https://github.com/duckdblabs/duckdb-internal/issues/4524.

I've duplicated many old lambda tests in a subdirectory and enabled allow_deprecated_lambda_syntax. I've also updated the original tests to use the new syntax to find and solve any issues with it.

Great change, great work 👍

I would add some tests around mixing syntaxes AND around macro definitions (since those are not parser time but bind time).

Like:

CREATE OR REPLACE function transpose(lst) AS (
      SELECT list_transform(range(1, 1 + length(lst[1])),
          j -> list_transform(range(1, length(lst) + 1),
             LAMBDA i : lst[i][j]
          )
      )
  );
SELECT transpose('123');

(currently INTERNAL Error)

Plus there is also some possible problem with serialization, like:

carlo@ScroogeMcDuck duckdb % ./build/release/duckdb tmp_file.db
v1.3.0-dev2747 c1bc6aa827
Enter ".help" for usage hints.
D CREATE OR REPLACE function transpose(l) AS (
          SELECT list_transform(asd:=range(1, 1 + length(l[1])),
            i := j ->list_transform(range(1, length(l) + 1),
             l :=    i-> l[i:12]
              )
          )
      );
D 
D 
carlo@ScroogeMcDuck duckdb % duckdb tmp_file.db                
Error: unable to open database "tmp_file.db": Serialization Error: Failed to deserialize: expected end of object, but found field id: 202
carlo@ScroogeMcDuck duckdb % ./build/release/duckdb tmp_file.db
v1.3.0-dev2747 c1bc6aa827
Enter ".help" for usage hints.

(unsure what's the path here, does this need to be actually visible to storage serialization?)

And I would say : is somehow loaded already:

WITH tbl AS (SELECT l :'lambda')
  FROM tbl
      SELECT list_transform(range(1, length(l) + 1), a := lambda i : l[0:i]);

This makes me a tiny bit sad, I liked the -> syntax and found it more readable and less esoteric than lambda. On the other hand, I'm excited for more reliable binding with macros. Do you know if this fixes this discussion?

@carlopi, the output of this one looks correct to me - I am not sure what the a := ... does, there? Is that a named parameter wrapping the expression?

WITH tbl AS (SELECT l :'lambda')
  FROM tbl
      SELECT list_transform(range(1, length(l) + 1), a := lambda i : l[0:i]);

(unsure what's the path here, does this need to be actually visible to storage serialization?)

If we don't expose it, then we'll get different results for the verification runs.

@taniabogatsch a := is a named parameter, we use that syntax for named parameters in macros. I didn't realise it also works positionally for scalar functions that do not have any named parameters. It just gives the value the alias "a" internally.

@carlopi, I added some changes and all of these should pass now. Thanks a lot for your detailed comments!

statement error
CREATE OR REPLACE function transpose(lst) AS (
	SELECT list_transform(range(1, 1 + length(lst[1])),
		j -> list_transform(range(1, length(lst) + 1),
			LAMBDA i : lst[i][j]
		)
	)
);
----
<REGEX>:Binder Error.*Deprecated lambda arrow.*

statement error
CREATE OR REPLACE FUNCTION transpose(lst) AS (
	SELECT list_transform(range(1, 1 + length(lst[1])),
		j -> list_transform(range(1, length(lst) + 1),
			i -> lst[i][j]
		)
	)
);
----
<REGEX>:Binder Error.*Deprecated lambda arrow.*

statement ok
SET allow_deprecated_lambda_syntax=true

statement ok
CREATE OR REPLACE FUNCTION transpose(lst) AS (
	SELECT list_transform(range(1, 1 + length(lst[1])),
		j -> list_transform(range(1, length(lst) + 1),
			i -> lst[i][j]
		)
	)
);


statement ok
ATTACH '__TEST_DIR__/deprecated_syntax.db';

statement ok
CREATE OR REPLACE FUNCTION deprecated_syntax.transpose(lst) AS (
	SELECT list_transform(range(1, 1 + length(lst[1])),
		j -> list_transform(range(1, length(lst) + 1),
			i -> lst[i][j]
		)
	)
);

statement ok
DETACH deprecated_syntax;

statement ok
ATTACH '__TEST_DIR__/deprecated_syntax.db';

@eaubin - I haven't looked into that discussion, yet. But I am planning to fix the remaining open issues around lambda binding after this has been merged.

@hmeriann, I adjusted the regression tests to have the new syntax. From my understanding, they will break for this PR, and once merged, work again?

@taniabogatsch yes, this sounds right! Thank you for letting me know!

Great on the bind-time checks!!

If we don't expose it, then we'll get different results for the verification runs.

I am not sure I understand, I was thinking that if this is checked at creation, then stuff should just work from there (either 1 more or both are allowed, but downstream it does not matter and whatever entered the system has to keep working).

To me exposing this to serialization adds complexity for future us /adds failures mode I am not really sure we are interested in.

I am not sure of the details, could you expand on what fails differently?
I am not sure of the details, but I think cutting storage layer would be great here.

this is checked at creation, then stuff should just work from there

It is checked during the binding phase, which is after we've created a ParsedExpression with the Transformer.
I could move this check into the Transformer by propagating allow_deprecated_lambda_syntax to the ParserOptions. But then we still have the problem that once we allow both the new and old syntax, we have (if we drop the serialisation of the deprecated boolean) no way to say which one it was after serialisation/when calling ToString().

Currently, the deprecated flag lives in the LambdaExpression. If we do not add it to the serialisation code, then we need to decide on a default value. Which means that a deprecated expression can become a non-deprecated expression, and the other way around.

And if we decide to not have the deprecated flag in the LambdaExpression at all (which would solve both serialisation and default value), then I don't know how to solve ToString():

string LambdaExpression::ToString() const {
	if (deprecated) {
		return "(" + lhs->ToString() + " -> " + expr->ToString() + ")";
	}
	return "(LAMBDA " + lhs->ToString() + " : " + expr->ToString() + ")";
}

Because depending on which representation we choose, we either 1. start throwing invalid syntax for originally valid expressions, or turn a JSON arrow into a lambda expression...

Yes that will work as well (spaces are nops in SQL). @taniabogatsch maybe we can update the tests to follow the syntax commonly used in Python instead? (e.g. lambda x: x + 1 - no caps and no space before the colon).

I've implemented all suggestions and review requests, so this is ready for review again.
I'll adjust the PR description shortly.

The regression failure is expected, and the Python failure seems unrelated.

Thanks!

Nightly Regression had failed with syntax error - it should go away tomorrow