Skip to content

Drain Parquet writer tasks before returning failures - #167

Merged
osipovartem merged 2 commits into
df55-physical-udf-instancefrom
fix/parquet-writer-quiescence
Oct 26, 2026
Merged

osipovartem merged 2 commits into
df55-physical-udf-instancefrom
fix/parquet-writer-quiescence

Conversation

@osipovartem

@osipovartem osipovartem commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Observe writer completion while the demux channel remains open, so a failed writer cannot leave the sink waiting indefinitely for more input.
  • Abort and join the demux and remaining writers before returning a setup or writer error. This provides the quiescence required by Iceberg cleanup of failed writes.
  • Add a regression test with an open demux channel, one failing writer, and one waiting writer.

Verification

  • Focused cargo +1.97.0 test -p datafusion-datasource-parquet --lib writer_error_waits_for_other_writers_to_stop: 1 passed locally after the Clippy fix.
  • Targeted Rust 1.97 Clippy (-D warnings), Rust 1.94 cargo check -p datafusion (MSRV), cargo +1.97.0 fmt --all -- --check, and git diff --check passed locally.
  • Independent read-only review of the final let-chain edit approved it without blocking findings.
  • Rustice integration using the preceding writer-quiescence SHA 7955d2650 passed the 541-file, 4195-case strict SQLLogicTest corpus. The final 11874e4ba commit only collapses two behavior-equivalent conditions for Clippy; Rustice now pins that final SHA.

Performance and dependency

  • Compared the preceding writer-quiescence SHA 7955d2650 with base 6770c7c34 using the same release-mode DataFusion harness; final 11874e4ba only changes behavior-equivalent let-chain syntax: 1,048,576 rows, one or 32 local Parquet files, five warmups and 20 samples per run, 15 alternating A/B pairs. Median paired change was +1.08% for one file (about 0.25 ms) and +1.58% for 32 files (about 1.6 ms). The shorter 131,072-row run was noisier (+4.32% and +0.1% over 20 pairs); the patched single-file run was faster in 9/20 pairs.
  • This small measured local-write overhead is accepted to guarantee that failed writes stop before Iceberg removes uncommitted files. Coordination is per file/task completion, not per row; remote object-store performance was not measured.
  • Independent read-only review approved the change without blocking findings. Remove uncommitted Iceberg data files after failed writes iceberg-rust#138 must only be consumed together with this writer-quiescence revision.

CI notes

  • The MSRV job failed before compilation because cargo-msrv received HTTP 429 while fetching the Rust changelog; local Rust 1.94 compilation passed.
  • The security audit reports 15 advisories in pre-existing Cargo.lock dependencies; this PR changes neither dependencies nor Cargo.lock.

@osipovartem
osipovartem marked this pull request as ready for review October 26, 2026 22:07
@osipovartem
osipovartem merged commit 49e3f54 into df55-physical-udf-instance Oct 26, 2026
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant