feat: implement correct logic for nested lambdas and more complex lambda expressions #7056
Conversation
stevenpyzhang
force-pushed
the
nested-lambdas-patch
branch
2 times, most recently
from
7b2170e to
56eaa8f
Compare
stevenpyzhang
changed the title
| final boolean hasLambda = node.hasLambdaFunctionCallArguments(); | ||
| for (final Expression argExpr : node.getArguments()) { | ||
| final TypeContext childContext = context.getCopy(); | ||
| final TypeContext childContext; |
There was a problem hiding this comment.
Better add some comments for other readers why we are doing this: since the context need to be shared across function args that involves lambdas, blah blah.
ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/CodeGenRunner.java
Outdated
Show resolved
Hide resolved
| final FunctionName functionName = node.getName(); | ||
|
|
||
| final TypeContext contextCopy = context.getCopy(); |
There was a problem hiding this comment.
nit: maybe rename to lambdaSharedContext?
There was a problem hiding this comment.
took me a while as well to understand what was going on. I'd name them like this:
// this is the one that's passed in, it represents the context at the time the parent called this method
TypeContext parentTypeContext;
// this is a copy of the parent type context, additionally populated with the result of visiting non-lambda
// expressions - it starts as a copy of the parent typeContext
TypeContext currentTypeContext;
// a copy of either parent or current type context to be passed to the child - in the case of lambdas
// we pass in the parent type context without the result of resolving the current context because
// there may be valid overlapping lambda parameter names
TypeContext childContext;
| final SqlType resolvedArgType = | ||
| expressionTypeManager.getExpressionSqlType(argExpr, childContext); | ||
| process(argExpr, context.getCopy()); | ||
| expressionTypeManager.getExpressionSqlType(argExpr, childContext.getCopy()); |
There was a problem hiding this comment.
We are a bit duplicate copying context here. Maybe we can wrap the logic of
final TypeContext childContext;
if (argExpr instanceof LambdaFunctionCall) {
childContext = contextCopy.getCopy();
} else {
childContext = context.getCopy();
}
as a argTypeContext(argExpr, parentContext, lambdaSharedContext), which would be called for both expressionTypeManager#getExpressionSqlType and process? Ditto on other classes.
ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java
Outdated
Show resolved
Hide resolved
stevenpyzhang
force-pushed
the
nested-lambdas-patch
branch
2 times, most recently
from
8225a82 to
d93cd2e
Compare
|
PR is blocked on #7093 right now |
stevenpyzhang
force-pushed
the
nested-lambdas-patch
branch
3 times, most recently
from
3aed980 to
e881860
Compare
| "name": "apply transform lambda function to array", | ||
| "statements": [ | ||
| "CREATE STREAM TEST (ID BIGINT KEY, VALUE MAP<STRING, ARRAY<INT>>) WITH (kafka_topic='test_topic', value_format='AVRO');", | ||
| "CREATE STREAM OUTPUT as SELECT ID, TRANSFORM(TRANSFORM(VALUE, (x,y) => x, (x,y) => FIlTER(y, z => z IS NOT NULL)), (x,y) => UCASE(x) , (k,v) => ARRAY_MAX(v)) as FILTERED_TRANSFORMED from TEST emit changes;" |
There was a problem hiding this comment.
it might be more interesting to filter something like z => z < 5 - otherwise ARRAY_MAX([2,null,5]) might just be returning 5 anyway, right?
|
|
||
| // Then | ||
| assertThat( | ||
| javaExpression, equalTo("((String) function_0.evaluate(COL4, COL1, new BiFunction() {\n" |
There was a problem hiding this comment.
personally, I don't think these are valuable to check in (though obviously great for development) - the important part is that the results are the same and the QTT does that. We might want to change the code that we generate and it would have no impact other than cause our tests to fail 😢
stevenpyzhang
force-pushed
the
nested-lambdas-patch
branch
from
be00e91 to
40c25bf
Compare
stevenpyzhang
force-pushed
the
nested-lambdas-patch
branch
from
40c25bf to
60ce3df
Compare
Description
When processing a function call, we only want to use the context with the input type list populated for the LambdaFunctionCalls since those are where we need the input type list for mapping arguments. For other FunctionCall argument expressions, we need to pass in the original context that we arrived at the node with.
We also need to be passing copies of the contexts in order to prevent corruption when mapping the input type to the lambda arguments from other subtrees.
Testing done
Unit tests so far, QTT test coming soon.
Reviewer checklist