(datafusion) 01/01: Explicitly declare spill codec dependency in `physical-plan` (#21917)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-21917-7d107f09ba0e980e8885b60187a9621726c941d4
in repository https://gitbox.apache.org/repos/asf/datafusion.git

commit c134a848289cc465b0c4c3dc8be076cc10e299db
Author: kosiew <ko...@gmail.com>
AuthorDate: Tue May 5 10:44:48 2026 +0800

    Explicitly declare spill codec dependency in `physical-plan` (#21917)
    
    ## Which issue does this PR close?
    
    * Closes #21914.
    
    ---
    
    ## Rationale for this change
    
    The spill subsystem implicitly relied on workspace-level Cargo feature
    unification for codec support (e.g., `arrow-ipc/zstd`). This created a
    fragile setup where changes in unrelated dependencies or feature flags
    could silently break spill compression.
    
    This PR establishes an explicit, crate-local contract for codec
    availability, improving correctness, maintainability, and visibility of
    dependencies.
    
    ---
    
    ## What changes are included in this PR?
    
    * Add an explicit `arrow-ipc` dependency with `lz4` and `zstd` features
    in `datafusion-physical-plan` to ensure required codecs are always
    available.
    * Document the codec contract directly in `IPCStreamWriter::new`,
    clarifying expectations and failure modes.
    * Update compression handling:
    
    * Convert `SpillCompression` into `Option<CompressionType>` before
    passing to `try_with_compression`.
    * Rename parameter `compression_type` → `spill_compression` for clarity.
    * Add `arrow-ipc` as a dependency in the workspace lockfile.
    * Minor cleanup:
    
      * Adjust `cargo-machete` ignored field to array format.
      * Improve comments around `serde_json` usage.
    
    ---
    
    ## Are these changes tested?
    
    No new tests are included in this PR.
    
    The changes rely on existing spill-related tests. No modifications to
    tests were made in this diff.
    
    ---
    
    ## Are there any user-facing changes?
    
    No direct user-facing changes.
    
    This PR improves internal reliability and makes spill compression
    behavior more robust against dependency or feature configuration
    changes.
    
    ---
    
    ## LLM-generated code disclosure
    
    This PR includes LLM-generated code and comments. All LLM-generated
    content has been manually reviewed and tested.
---
 Cargo.lock                                |  1 +
 datafusion-cli/Cargo.toml                 |  6 +++---
 datafusion/physical-plan/Cargo.toml       |  4 ++++
 datafusion/physical-plan/src/spill/mod.rs | 17 +++++++++++++++--
 datafusion/sqllogictest/Cargo.toml        |  6 +++---
 5 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index af52588e53..833839d57e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2416,6 +2416,7 @@ version = "53.1.0"
 dependencies = [
  "arrow",
  "arrow-data",
+ "arrow-ipc",
  "arrow-ord",
  "arrow-schema",
  "async-trait",
diff --git a/datafusion-cli/Cargo.toml b/datafusion-cli/Cargo.toml
index 414b8c6444..baf8e2c297 100644
--- a/datafusion-cli/Cargo.toml
+++ b/datafusion-cli/Cargo.toml
@@ -82,7 +82,7 @@ testcontainers-modules = { workspace = true, features = ["minio"] }
 # feature unification with dependencies
 serde_json = { workspace = true, features = ["preserve_order"] }
 
-# Required because we pull serde_json with a feature to get consistent pg display,
-# but its not directly used.
+# serde_json is pulled with a feature to get
+# consistent pg display, but is not directly used in this crate.
 [package.metadata.cargo-machete]
-ignored = "serde_json"
+ignored = ["serde_json"]
diff --git a/datafusion/physical-plan/Cargo.toml b/datafusion/physical-plan/Cargo.toml
index 374fc275a0..7c52ee6e17 100644
--- a/datafusion/physical-plan/Cargo.toml
+++ b/datafusion/physical-plan/Cargo.toml
@@ -49,6 +49,10 @@ name = "datafusion_physical_plan"
 [dependencies]
 arrow = { workspace = true }
 arrow-data = { workspace = true }
+# Spill IPC writes require lz4 and zstd codec support. Keep these features in
+# sync with the SpillCompression variants in datafusion-common so codec
+# availability is explicit in the crate that owns spill handling.
+arrow-ipc = { workspace = true, features = ["lz4", "zstd"] }
 arrow-ord = { workspace = true }
 arrow-schema = { workspace = true }
 async-trait = { workspace = true }
diff --git a/datafusion/physical-plan/src/spill/mod.rs b/datafusion/physical-plan/src/spill/mod.rs
index 51e59318e2..3c95a1da5b 100644
--- a/datafusion/physical-plan/src/spill/mod.rs
+++ b/datafusion/physical-plan/src/spill/mod.rs
@@ -48,6 +48,7 @@ use arrow::ipc::{
 };
 use arrow::record_batch::RecordBatch;
 use arrow_data::ArrayDataBuilder;
+use arrow_ipc::CompressionType;
 
 use datafusion_common::config::SpillCompression;
 use datafusion_common::{DataFusionError, Result, exec_datafusion_err, exec_err};
@@ -290,10 +291,21 @@ struct IPCStreamWriter {
 
 impl IPCStreamWriter {
     /// Create new writer
+    ///
+    /// # Codec contract
+    ///
+    /// `arrow-ipc` must be compiled with the `lz4` and `zstd` features
+    /// (declared explicitly in `datafusion-physical-plan/Cargo.toml`). If
+    /// those features are absent, `try_with_compression` will return an
+    /// error at runtime for [`SpillCompression::Lz4Frame`] and
+    /// [`SpillCompression::Zstd`] variants. The Cargo dependency keeps this
+    /// contract local and build-visible during Cargo feature resolution,
+    /// rather than relying solely on workspace-level feature unification;
+    /// see #21917.
     pub fn new(
         path: &Path,
         schema: &Schema,
-        compression_type: SpillCompression,
+        spill_compression: SpillCompression,
     ) -> Result<Self> {
         let file = File::create(path).map_err(|e| {
             exec_datafusion_err!("(Hint: you may increase the file descriptor limit with shell command 'ulimit -n 4096') Failed to create partition file at {path:?}: {e:?}")
@@ -308,7 +320,8 @@ impl IPCStreamWriter {
         let alignment = get_max_alignment_for_schema(schema);
         let mut write_options =
             IpcWriteOptions::try_new(alignment, false, metadata_version)?;
-        write_options = write_options.try_with_compression(compression_type.into())?;
+        let compression_type = Option::<CompressionType>::from(spill_compression);
+        write_options = write_options.try_with_compression(compression_type)?;
 
         let writer = StreamWriter::try_new_with_options(file, schema, write_options)?;
         Ok(Self {
diff --git a/datafusion/sqllogictest/Cargo.toml b/datafusion/sqllogictest/Cargo.toml
index 1159b7f3b7..e2ffe1415a 100644
--- a/datafusion/sqllogictest/Cargo.toml
+++ b/datafusion/sqllogictest/Cargo.toml
@@ -94,7 +94,7 @@ harness = false
 name = "sqllogictests"
 path = "bin/sqllogictests.rs"
 
-# Required because we pull serde_json with a feature to get consistent pg display,
-# but its not directly used.
+# serde_json is pulled with a feature to get
+# consistent pg display, but is not directly used in this crate.
 [package.metadata.cargo-machete]
-ignored = "serde_json"
+ignored = ["serde_json"]


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org