Copilot commented on code in PR #18672:
URL: https://github.com/apache/datafusion/pull/18672#discussion_r2547691337
##########
datafusion/ffi/README.md:
##########
@@ -101,6 +101,36 @@ In this crate we have a variety of structs which closely
mimic the behavior of
their internal counterparts. To see detailed notes about how to use them, see
the example in `FFI_TableProvider`.
+## Library Marker ID
+
+When reviewing the code, many of the structs in this crate contain a call to
+a `library_maker_id`. The purpose of this call is to determine if a library is
Review Comment:
Corrected spelling of 'library_maker_id' to 'library_marker_id'.
```suggestion
a `library_marker_id`. The purpose of this call is to determine if a library
is
```
##########
datafusion/ffi/src/plan_properties.rs:
##########
@@ -314,19 +318,43 @@ mod tests {
let _ = eqp.reorder([PhysicalSortExpr::new_default(
datafusion::physical_plan::expressions::col("a", &schema)?,
)]);
- let original_props = PlanProperties::new(
+ Ok(PlanProperties::new(
eqp,
Partitioning::RoundRobinBatch(3),
EmissionType::Incremental,
Boundedness::Bounded,
- );
+ ))
+ }
- let local_props_ptr = FFI_PlanProperties::from(&original_props);
+ #[test]
+ fn test_round_trip_ffi_plan_properties() -> Result<()> {
+ let original_props = create_test_props()?;
+ let mut local_props_ptr = FFI_PlanProperties::from(&original_props);
+ local_props_ptr.library_marker_id = crate::mock_foreign_marker_id;
let foreign_props: PlanProperties = local_props_ptr.try_into()?;
assert_eq!(format!("{foreign_props:?}"),
format!("{original_props:?}"));
Ok(())
}
+
+ #[test]
+ fn test_ffi_execution_plan_local_bypass() -> Result<()> {
+ let props = create_test_props()?;
+
+ let ffi_plan = FFI_PlanProperties::from(&props);
+
+ // Verify local libraries
+ let foreign_plan: PlanProperties = ffi_plan.try_into()?;
+ assert_eq!(format!("{foreign_plan:?}"), format!("{:?}", foreign_plan));
+
+ // Verify different library markers still can produce identical
properties
+ let mut ffi_plan = FFI_PlanProperties::from(&props);
+ ffi_plan.library_marker_id = crate::mock_foreign_marker_id;
+ let foreign_plan: PlanProperties = ffi_plan.try_into()?;
+ assert_eq!(format!("{foreign_plan:?}"), format!("{:?}", foreign_plan));
Review Comment:
The assertion compares `foreign_plan` to itself, which will always pass.
This should likely compare against `props` to verify that the properties are
preserved.
```suggestion
assert_eq!(format!("{foreign_plan:?}"), format!("{props:?}"));
// Verify different library markers still can produce identical
properties
let mut ffi_plan = FFI_PlanProperties::from(&props);
ffi_plan.library_marker_id = crate::mock_foreign_marker_id;
let foreign_plan: PlanProperties = ffi_plan.try_into()?;
assert_eq!(format!("{foreign_plan:?}"), format!("{props:?}"));
```
--
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]
