Skip to content

Commit 34481e8

Browse files
alxmrsclaude
andcommitted
Upgrade to DataFusion 52: fix FFI, implement collect() TODO (#107).
- Fix FFI_TableProvider::new() → new_with_ffi_codec(): DataFusion 52 changed the table provider FFI signature to require a LogicalExtensionCodec. The SessionContext now passes itself as a `session` arg to __datafusion_table_provider__; we extract the codec via __datafusion_logical_extension_codec__() following the official datafusion-python v52 FFI example. - Add ffi_logical_codec_from_pycapsule() helper (mirrors examples/datafusion-ffi-example/src/utils.rs). - Remove FFI workaround notes and TODO(#107): the collect() bug in DataFusion v51 is fixed in v52. Update test_aggregation_with_many_batches to use collect() directly to verify correctness of parallel aggregation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2de1342 commit 34481e8

3 files changed

Lines changed: 69 additions & 37 deletions

File tree

src/lib.rs

Lines changed: 63 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,7 @@
2727
//! ## Parallel Execution
2828
//!
2929
//! Each xarray chunk becomes a separate partition, enabling parallel execution across
30-
//! multiple cores. Due to a bug in DataFusion v51.0.0's `collect()` method, aggregation
31-
//! queries should use `to_arrow_table()` instead to ensure complete results.
32-
//! TODO(#107): Upgrading to the latest datafusion-python (52+) should fix this.
30+
//! multiple cores.
3331
//!
3432
//! ## Filter Pushdown (Partition Pruning)
3533
//!
@@ -66,10 +64,10 @@ use datafusion::logical_expr::{
6664
use datafusion::physical_plan::stream::RecordBatchStreamAdapter;
6765
use datafusion::physical_plan::streaming::PartitionStream;
6866
use datafusion::physical_plan::{ExecutionPlan, SendableRecordBatchStream};
67+
use datafusion_ffi::proto::logical_extension_codec::FFI_LogicalExtensionCodec;
6968
use datafusion_ffi::table_provider::FFI_TableProvider;
7069
use pyo3::prelude::*;
7170
use pyo3::types::{PyCapsule, PyList};
72-
use tokio::runtime::Handle;
7371

7472
// ============================================================================
7573
// Partition Metadata Types for Filter Pushdown
@@ -788,6 +786,58 @@ impl PartitionStream for PyArrowStreamPartition {
788786
}
789787
}
790788

789+
// ============================================================================
790+
// FFI Helpers
791+
// ============================================================================
792+
793+
/// Extract an `FFI_LogicalExtensionCodec` from a Python session object.
794+
///
795+
/// DataFusion 52 passes the `SessionContext` to `__datafusion_table_provider__`
796+
/// so that the provider can obtain the codec needed for physical-plan
797+
/// serialisation across the FFI boundary. The session exposes this via
798+
/// `__datafusion_logical_extension_codec__()`, which returns a PyCapsule
799+
/// named `"datafusion_logical_extension_codec"`.
800+
///
801+
/// Mirrors the helper in the official datafusion-python FFI example
802+
/// (`examples/datafusion-ffi-example/src/utils.rs`).
803+
fn ffi_logical_codec_from_pycapsule(
804+
session: Bound<'_, PyAny>,
805+
) -> PyResult<FFI_LogicalExtensionCodec> {
806+
let attr = "__datafusion_logical_extension_codec__";
807+
let capsule = if session.hasattr(attr)? {
808+
session.getattr(attr)?.call0()?
809+
} else {
810+
session
811+
};
812+
813+
let capsule = capsule.downcast::<PyCapsule>().map_err(|e| {
814+
pyo3::exceptions::PyValueError::new_err(format!(
815+
"session did not produce a PyCapsule for the logical extension codec: {e}"
816+
))
817+
})?;
818+
819+
let capsule_name = capsule.name()?.ok_or_else(|| {
820+
pyo3::exceptions::PyValueError::new_err(
821+
"datafusion_logical_extension_codec PyCapsule has no name set",
822+
)
823+
})?;
824+
let capsule_name = capsule_name.to_str()?;
825+
if capsule_name != "datafusion_logical_extension_codec" {
826+
return Err(pyo3::exceptions::PyValueError::new_err(format!(
827+
"expected capsule name 'datafusion_logical_extension_codec', got '{capsule_name}'"
828+
)));
829+
}
830+
831+
// SAFETY: The capsule was produced by datafusion-python and contains a
832+
// valid FFI_LogicalExtensionCodec (#[repr(C)] StableAbi struct).
833+
let codec = unsafe { capsule.reference::<FFI_LogicalExtensionCodec>() };
834+
Ok(codec.clone())
835+
}
836+
837+
// ============================================================================
838+
// Python-visible Table Class
839+
// ============================================================================
840+
791841
/// A lazy table provider that wraps Python stream factory functions.
792842
///
793843
/// This class implements the `__datafusion_table_provider__` protocol, allowing
@@ -802,11 +852,6 @@ impl PartitionStream for PyArrowStreamPartition {
802852
/// SQL filters on dimension columns (time, lat, lon, etc.) automatically prune
803853
/// partitions that can't contain matching rows when metadata is supplied.
804854
///
805-
/// # Note
806-
///
807-
/// Due to a bug in DataFusion v51.0.0's `collect()` method, use `to_arrow_table()`
808-
/// instead for aggregation queries to ensure complete results.
809-
///
810855
/// # Example
811856
///
812857
/// ```python
@@ -829,6 +874,7 @@ impl PartitionStream for PyArrowStreamPartition {
829874
/// ctx.register_table("air", table)
830875
/// result = ctx.sql("SELECT AVG(air) FROM air WHERE time > 500000000").to_arrow_table()
831876
/// ```
877+
832878
#[pyclass(name = "LazyArrowStreamTable")]
833879
struct LazyArrowStreamTable {
834880
/// The underlying table provider with pruning support
@@ -902,27 +948,22 @@ impl LazyArrowStreamTable {
902948
///
903949
/// This method is called by DataFusion's `register_table()` to get a
904950
/// foreign table provider that can be used in queries.
951+
///
952+
/// In DataFusion 52+, the caller passes `session` (a `SessionContext`)
953+
/// so that the provider can access task-context and codec information
954+
/// needed for physical plan serialisation across the FFI boundary.
905955
fn __datafusion_table_provider__<'py>(
906956
&self,
907957
py: Python<'py>,
958+
session: Bound<'py, PyAny>,
908959
) -> PyResult<Bound<'py, PyCapsule>> {
909-
// Create the FFI table provider
960+
let codec = ffi_logical_codec_from_pycapsule(session)?;
961+
910962
let provider: Arc<dyn TableProvider + Send> = self.table.clone();
911963

912-
// Try to get the current tokio runtime handle (available when called from DataFusion context)
913-
let runtime = Handle::try_current().ok();
964+
let ffi_provider = FFI_TableProvider::new_with_ffi_codec(provider, true, None, codec);
914965

915-
// Create FFI wrapper with filter pushdown ENABLED
916-
let ffi_provider = FFI_TableProvider::new(
917-
provider, true, // can_support_pushdown_filters = ENABLED
918-
runtime,
919-
);
920-
921-
// Create the capsule name
922966
let name = CString::new("datafusion_table_provider").unwrap();
923-
924-
// Create the PyCapsule without a destructor closure
925-
// The PyCapsule takes ownership of the FFI_TableProvider
926967
PyCapsule::new(py, ffi_provider, Some(name))
927968
}
928969

xarray_sql/reader.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -212,11 +212,6 @@ def read_xarray_table(
212212
213213
Supported operators: =, <, >, <=, >=, BETWEEN, IN, AND, OR.
214214
215-
Note:
216-
Due to a bug in DataFusion v51.0.0's collect() method, use
217-
`to_arrow_table()` instead of `collect()` for aggregation queries
218-
to ensure complete results. This should be fixed in datafusion-python 52+.
219-
220215
Args:
221216
ds: An xarray Dataset. All data_vars must share the same dimensions.
222217
chunks: Xarray-like chunks specification. If not provided, uses

xarray_sql/reader_test.py

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -739,12 +739,8 @@ def test_aggregation_with_many_batches(self):
739739
"""Verify aggregation queries work correctly with many batches.
740740
741741
GROUP BY queries require processing all data, making them a good
742-
test for streaming behavior.
743-
744-
Note: We use to_arrow_table() instead of collect() due to a bug in
745-
DataFusion v51.0.0 where collect() returns partial results for
746-
parallel aggregation queries.
747-
# TODO(#107): Upgrade to latest datafusion-python, which has the fix.
742+
test for streaming behavior. Uses collect() to verify that parallel
743+
aggregation returns complete results (fixed in DataFusion 52+).
748744
"""
749745
np.random.seed(789)
750746
time_coord = pd.date_range("2020-01-01", periods=120, freq="h")
@@ -770,11 +766,11 @@ def test_aggregation_with_many_batches(self):
770766
ctx = SessionContext()
771767
ctx.register_table("test_table", table)
772768

773-
# GROUP BY requires scanning all data
774-
# Use to_arrow_table() to avoid DataFusion collect() bug
775-
result = ctx.sql(
769+
# GROUP BY requires scanning all data; collect() must return complete results
770+
batches = ctx.sql(
776771
"SELECT lat, AVG(temperature) as avg_temp FROM test_table GROUP BY lat"
777-
).to_arrow_table()
772+
).collect()
773+
result = pa.Table.from_batches(batches)
778774

779775
# Should have result for each lat value
780776
df = result.to_pandas()

0 commit comments

Comments
 (0)