kosiew commented on code in PR #24218:
URL: https://github.com/apache/datafusion/pull/24218#discussion_r3819989376
##########
datafusion/ffi/src/tests/mod.rs:
##########
@@ -120,6 +120,8 @@ pub struct ForeignLibraryModule {
pub create_exec_with_statistics: extern "C" fn() -> FFI_ExecutionPlan,
+ pub create_exec_with_byte_metrics: extern "C" fn() -> FFI_ExecutionPlan,
Review Comment:
`ForeignLibraryModule` is public and `#[repr(C)]`, so adding a field here
shifts every field that follows it. A consumer compiled against the previous
layout could then call the wrong factory function pointers. This also appears
to be exactly what the `constructible_struct_adds_field` SemVer check is
reporting.
Even though this is test support, it is exposed through the public
`integration-tests` feature and is the cross-library ABI surface these tests
rely on. Could we avoid extending this struct in the current release API? A
separate exported symbol or loading path for this test factory would keep the
existing layout intact.
##########
datafusion/ffi/tests/ffi_execution_plan.rs:
##########
@@ -68,6 +68,39 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_ffi_execution_plan_byte_metrics_cross_library() -> Result<(),
DataFusionError>
+ {
+ let module = get_module()?;
+ let plan = (module.create_exec_with_byte_metrics)();
+ let plan: Arc<dyn ExecutionPlan> = (&plan).try_into()?;
+ assert!(plan.is::<ForeignExecutionPlan>());
+
+ // metrics() crosses the FFI boundary for real here: `plan` is a
+ // ForeignExecutionPlan backed by a separately loaded copy of this
+ // same cdylib, so this call marshals a MetricsSet containing
+ // MetricValue::BytesCount/BytesGauge through FFI_MetricsSet across
+ // that boundary - not just the in-process From conversions covered
+ // by physical_expr::metrics's roundtrip tests.
+ let metrics = plan.metrics().expect("plan should report metrics");
+ let rendered: Vec<String> = metrics.iter().map(|m|
m.to_string()).collect();
Review Comment:
Could we verify the metric variants explicitly here as well? Comparing the
rendered strings confirms the display output, but it does not prove that the
`BytesCount` and `BytesGauge` discriminants survived the ABI round trip.
I think this test should match `metric.value()` against
`MetricValue::BytesCount { name, count }` and `MetricValue::BytesGauge { name,
gauge }`, then assert both the names and numeric values. Keeping the display
assertions too would still be useful.
--
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]
