kosiew commented on code in PR #26758:
URL: https://github.com/apache/datafusion/pull/26758#discussion_r4562338032
##########
datafusion/proto/tests/cases/stack_safety.rs:
##########
@@ -97,3 +101,45 @@ fn
deeply_nested_logical_plan_serialization_uses_a_growable_stack() {
// exercising the `recursive` stack-growth checkpoint.
run_in_child(GROWABLE_STACK_TEST_NAME, 2_000);
}
+
+// Logical plan deserialization (#26737): nested nodes go through
+// `try_into_logical_plan` recursively, so a deep plan needs the same
+// stack-growth checkpoint as serialization.
+#[cfg(feature = "recursive_protection")]
+mod deserialization {
+ use datafusion_execution::TaskContext;
+ use datafusion_proto::logical_plan::{AsLogicalPlan,
DefaultLogicalExtensionCodec};
+ use datafusion_proto::protobuf::LogicalPlanNode;
+
+ use super::{deeply_aliased_plan, run_in_child_with};
+
+ const TEST_NAME: &str =
"cases::stack_safety::deserialization::deeply_nested_logical_plan_deserialization_uses_a_growable_stack";
+
+ fn deserialize_on_two_mib_stack(alias_depth: usize) {
+ let plan = deeply_aliased_plan(alias_depth);
+ std::thread::Builder::new()
+ .name("two-megabyte-stack".into())
+ .stack_size(2 * 1024 * 1024)
+ .spawn(move || {
+ // Convert to and from the proto struct directly, skipping the
+ // byte encoding, so prost's decode recursion limit (100
levels)
+ // does not cap the depth.
+ let codec = DefaultLogicalExtensionCodec {};
+ let proto =
+ LogicalPlanNode::try_from_logical_plan(&plan,
&codec).unwrap();
+ proto
+ .try_into_logical_plan(&TaskContext::default(), &codec)
Review Comment:
Optional: Could we retain the decoded plan and iteratively verify all
`alias_depth` `SubqueryAlias` nodes, their names (`level_1999` through
`level_0`), and the `EmptyRelation` leaf? The current test is sufficient for
stack safety, but these checks would also confirm deep roundtrip preservation
without recursive equality.
##########
datafusion/proto/tests/cases/stack_safety.rs:
##########
@@ -97,3 +101,45 @@ fn
deeply_nested_logical_plan_serialization_uses_a_growable_stack() {
// exercising the `recursive` stack-growth checkpoint.
run_in_child(GROWABLE_STACK_TEST_NAME, 2_000);
}
+
+// Logical plan deserialization (#26737): nested nodes go through
+// `try_into_logical_plan` recursively, so a deep plan needs the same
+// stack-growth checkpoint as serialization.
+#[cfg(feature = "recursive_protection")]
Review Comment:
Optional follow-up: Could we add `cargo test -p datafusion-proto --features
recursive_protection --test proto_integration cases::stack_safety` to CI? The
workspace test configuration omits this feature, and the existing
recursive-protection job enables it only for `datafusion`, so neither proto
stack-safety test runs in ordinary CI. This is a pre-existing coverage gap, not
a blocker.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
