Add protobuf support for lambdas#22362
Conversation
| return Err(Error::General( | ||
| "Proto serialization error: Lambda not implemented".to_string(), | ||
| )); | ||
| Expr::HigherOrderFunction(HigherOrderFunction { func, args }) => { |
There was a problem hiding this comment.
super nit: instead of appending this, can we move this next to the rest of functions? (Scalar, Aggregate etc)
| HigherOrderUDFExprNode higher_order_udf_expr = 37; | ||
| Lambda lambda = 38; | ||
| LambdaVariable lambda_variable = 39; |
There was a problem hiding this comment.
super nit as well: Can we move this next to the other UDFs?
| expr_id, | ||
| expr_type: Some(protobuf::physical_expr_node::ExprType::LambdaVariable( | ||
| PhysicalLambdaVariableExprNode { | ||
| index: var.index() as u32, |
There was a problem hiding this comment.
for now we don't use this so it is always 0 no?
| protobuf::PhysicalHigherOrderUdfNode { | ||
| name: expr.name().to_string(), | ||
| args: serialize_physical_exprs(expr.args(), codec, proto_converter)?, | ||
| fun_definition: (!buf.is_empty()).then_some(buf), | ||
| }, |
There was a problem hiding this comment.
Shouldn't we encode the return_field instead of resolving it from the schema when decoding (like ScalarUDF)?
Not really sure what would be the exact disadvantage of keeping it at is, but wondering if it would be cleaner to make the return type explicit.
| let serialize_name = extract_function_name(&expr); | ||
| let deserialize_name = extract_function_name(&deserialize); | ||
|
|
||
| assert_eq!(serialize_name, deserialize_name); |
There was a problem hiding this comment.
should we assert directly on expr and deserialize instead? since the Hofs implement PartialEq I think it should work
assert_eq!(expr, deserialized_expr);
2 participants
Which issue does this PR close?
Part of #21172
Rationale for this change
Protobuf support wasn't implemented in main lambda PR to not make it even bigger
What changes are included in this PR?
Protobuf encoding and decoding (~1000 LOC in generated files, ~210 impl, ~400 tests)
Are these changes tested?
Unit tests, similar to the existing ones for scalar functions
Are there any user-facing changes?
None