You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kkrainov (via GitHub)" <gi...@apache.org> on 2026/05/24 21:05:07 UTC
[PR] refactor: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
kkrainov opened a new pull request, #22503:
URL: https://github.com/apache/datafusion/pull/22503
## Which issue does this PR close?
- Closes #22425
## Rationale for this change
This PR migrates InListExpr to use the new try_to_proto and try_from_proto hooks, following the modular serialization architecture established in #21835 . This moves serialization logic into the expression itself, improving encapsulation and decentralizing the datafusion-proto logic as part of the broader effort in #22418
## What changes are included in this PR?
- Implemented `PhysicalExpr::try_to_proto for InListExpr`.
- Implemented `InListExpr::try_from_proto` inherent method.
- Wired hooks in `from_proto.rs` and removed the central downcast arm in to_proto.rs.
- Added isolated unit tests in `in_list.rs` using mock drivers to verify roundtrips and error handling.
## Are these changes tested?
Yes, these changes are covered by both new and existing tests:
- New Unit Tests: Added mod `proto_tests` to in_list.rs using mock drivers to verify `try_to_proto` and `try_from_proto` in isolation. These cover successful roundtrips, incorrect node types, and missing required fields.
- Existing Integration Tests: Verified that the existing InList roundtrip tests in datafusion-proto continue to pass after removing the central downcast logic.
- Linting: Verified that the changes pass cargo `clippy --all-targets --all-features` with zero warnings.
## 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: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #22503:
URL: https://github.com/apache/datafusion/pull/22503
--
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: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
Posted by "kkrainov (via GitHub)" <gi...@apache.org>.
kkrainov commented on PR #22503:
URL: https://github.com/apache/datafusion/pull/22503#issuecomment-4533719954
I've refactored the unit tests in in_list.rs to align with the new testing patterns introduced in #22471 by @adriangb, utilizing the shared StubEncoder/StubDecoder utilities for Protobuf hook verification.
I've also added several new test cases:
* Rejection of incorrect node types (using UnreachableDecoder).
* Error propagation (using failing_on(N)).
* Missing field handling.
--
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: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #22503:
URL: https://github.com/apache/datafusion/pull/22503#discussion_r3298468500
##########
datafusion/proto/src/physical_plan/from_proto.rs:
##########
@@ -328,18 +328,7 @@ pub fn parse_physical_expr_with_converter(
proto_converter,
)?))
}
- ExprType::InList(e) => in_list(
- parse_required_physical_expr(
- e.expr.as_deref(),
- ctx,
- "expr",
- input_schema,
- proto_converter,
- )?,
- parse_physical_exprs(&e.list, ctx, input_schema, proto_converter)?,
Review Comment:
I wonder if we should port some of these utility methods onto `PhysicalExprEncodeCtx`:
- `parse_required_physical_expr`
- `parse_physical_exprs`
- `serialize_physical_exprs`
?
--
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: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #22503:
URL: https://github.com/apache/datafusion/pull/22503#discussion_r3298466758
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -3821,3 +3880,163 @@ mod tests {
Ok(())
}
}
+
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use super::*;
Review Comment:
`proto_tests` is declared *after* the `#[cfg(test)] mod tests` module. This will trigger Clippy's `items-after-test-module` lint (the file already has a `tests` module and it must remain the final item). Consider moving `proto_tests` inside the existing `tests` module (e.g., `#[cfg(feature = "proto")] mod proto_tests { ... }`) or otherwise ensuring `mod tests` is the last item in the file.
--
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: port InListExpr to use try_to_proto/try_from_proto hooks [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #22503:
URL: https://github.com/apache/datafusion/pull/22503#discussion_r3298620399
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -3821,3 +3880,163 @@ mod tests {
Ok(())
}
}
+
+#[cfg(all(test, feature = "proto"))]
+mod proto_tests {
+ use super::*;
Review Comment:
it didn't...
--
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