You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2026/08/29 20:07:32 UTC
[PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
buraksenn opened a new pull request, #24775:
URL: https://github.com/apache/datafusion/pull/24775
## Which issue does this PR close?
- Closes #24617.
## Rationale for this change
Proto hooks that access fields individually can silently omit newly added state. Exhaustive destructuring makes such omissions compile errors.
## What changes are included in this PR?
- Exhaustively destructure `LambdaExpr` and `LambdaVariable` in their encoders.
- Exhaustively destructure their protobuf nodes in the decoders.
- Document constructor-derived state and add focused proto hook tests.
The behavior and wire format are unchanged.
## Are these changes tested?
Covered by the added lambda proto hook tests:
```bash
cargo test -p datafusion-physical-expr --features proto lambda::proto_tests
cargo test -p datafusion-physical-expr --features proto lambda_variable::proto_tests
```
## Are there any user-facing changes?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24775:
URL: https://github.com/apache/datafusion/pull/24775
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24775:
URL: https://github.com/apache/datafusion/pull/24775#issuecomment-5464731509
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24775?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.24561%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.53%. Comparing base ([`61bf6b9`](https://app.codecov.io/gh/apache/datafusion/commit/61bf6b96cc07d6a0518014dec7fe86fbef76576e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`eea7b16`](https://app.codecov.io/gh/apache/datafusion/commit/eea7b166045ea47f84bd6f08a7a24ca681cd0df1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24775?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...n/physical-expr/src/expressions/lambda\_variable.rs](https://app.codecov.io/gh/apache/datafusion/pull/24775?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Flambda_variable.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9sYW1iZGFfdmFyaWFibGUucnM=) | 97.14% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24775?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-expr/src/expressions/lambda.rs](https://app.codecov.io/gh/apache/datafusion/pull/24775?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Flambda.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9sYW1iZGEucnM=) | 99.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24775?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24775 +/- ##
========================================
Coverage 81.52% 81.53%
========================================
Files 1123 1123
Lines 405970 406133 +163
Branches 405970 406133 +163
========================================
+ Hits 330978 331139 +161
- Misses 55627 55630 +3
+ Partials 19365 19364 -1
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24775?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24775:
URL: https://github.com/apache/datafusion/pull/24775#issuecomment-5535445623
🚀
@buraksenn
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24775:
URL: https://github.com/apache/datafusion/pull/24775#discussion_r3912248573
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -496,3 +509,148 @@ mod tests {
);
}
}
+
+/// Tests for the `try_to_proto` / `try_from_proto` hooks.
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use super::*;
+ use crate::proto_test_util::{
+ StubDecoder, StubEncoder, UnreachableDecoder, column_node,
+ };
+ use arrow::datatypes::Field;
+ use datafusion_common::DataFusionError;
+ use datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx;
+ use datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx;
+ use datafusion_proto_models::protobuf::{self, PhysicalExprNode};
+
+ /// A one-parameter lambda whose body references that parameter.
+ fn proto_lambda_fixture() -> LambdaExpr {
+ let v_field = Arc::new(Field::new("v", DataType::Int32, true));
+ LambdaExpr::try_new(
+ vec!["v".to_string()],
+ Arc::new(LambdaVariable::new(0, v_field)),
+ )
+ .unwrap()
+ }
+
+ fn proto_lambda_node(
+ params: Vec<String>,
+ body: Option<Box<PhysicalExprNode>>,
+ ) -> PhysicalExprNode {
+ PhysicalExprNode {
+ expr_id: None,
+ expr_type: Some(protobuf::physical_expr_node::ExprType::Lambda(Box::new(
+ protobuf::PhysicalLambdaExprNode { params, body },
+ ))),
+ }
+ }
+
+ #[test]
+ fn try_to_proto_encodes_lambda() {
Review Comment:
Could you consider adding one end-to-end nested lambda and lambda-variable proto roundtrip using the real encoder/decoder dispatch? The new unit tests do a good job of testing each hook in isolation, but since they use stubs they do not exercise the dispatcher path that connects these two variants. This is non-blocking, and the existing targeted hook and error coverage already looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(proto): destructure lambda expression serde hooks [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #24775:
URL: https://github.com/apache/datafusion/pull/24775#discussion_r3921296042
##########
datafusion/physical-expr/src/expressions/lambda.rs:
##########
@@ -496,3 +509,148 @@ mod tests {
);
}
}
+
+/// Tests for the `try_to_proto` / `try_from_proto` hooks.
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use super::*;
+ use crate::proto_test_util::{
+ StubDecoder, StubEncoder, UnreachableDecoder, column_node,
+ };
+ use arrow::datatypes::Field;
+ use datafusion_common::DataFusionError;
+ use datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx;
+ use datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx;
+ use datafusion_proto_models::protobuf::{self, PhysicalExprNode};
+
+ /// A one-parameter lambda whose body references that parameter.
+ fn proto_lambda_fixture() -> LambdaExpr {
+ let v_field = Arc::new(Field::new("v", DataType::Int32, true));
+ LambdaExpr::try_new(
+ vec!["v".to_string()],
+ Arc::new(LambdaVariable::new(0, v_field)),
+ )
+ .unwrap()
+ }
+
+ fn proto_lambda_node(
+ params: Vec<String>,
+ body: Option<Box<PhysicalExprNode>>,
+ ) -> PhysicalExprNode {
+ PhysicalExprNode {
+ expr_id: None,
+ expr_type: Some(protobuf::physical_expr_node::ExprType::Lambda(Box::new(
+ protobuf::PhysicalLambdaExprNode { params, body },
+ ))),
+ }
+ }
+
+ #[test]
+ fn try_to_proto_encodes_lambda() {
Review Comment:
Thanks for the review. I've added one e2e case specified above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org