fix(spark)!: reject lossy write conversions

[bvolpato]

Filesystem Overwrite, Ignore, and ErrorIfExists round-trip as append, while partitioned and bucketed file or Hive writes lose their layout. Incoming file UPDATE is converted into replacement of the target directory, and MODIFIED_RECORDS output is discarded.

Reject write modes, output modes, and layout metadata that the current conversion cannot preserve. Keep ordinary file append supported. Legacy APPEND_IF_EXISTS payloads remain supported only for targets known to be unpartitioned and unbucketed: FileHolder does not retain enough information to identify legacy payloads that lost layout metadata.

The spec defines create_mode for CTAS and is silent on INSERT. This binding leaves it unspecified on new file INSERT payloads and rejects legacy non-append values that its append-only conversion cannot honor. Supporting other save modes through ExtensionWrite(CTAS, create_mode) remains a separate change.

Summary by CodeRabbit

• New Features
  • Filesystem-based Parquet writes now support append operations, preserving existing rows while adding new data.
• Behavior Changes
  • Filesystem writes using overwrite, ignore, or error-if-exists modes are not supported.
  • Writes with static or conditional partitions, partition columns, or bucket specifications are not supported.
  • Importing filesystem write plans that use update or unsupported create modes is rejected.

BREAKING CHANGE: Spark write conversion now rejects partitioned or bucketed V1 file/Hive commands, non-append filesystem save modes, incoming file UPDATE, MODIFIED_RECORDS output, and legacy filesystem INSERT plans with non-append create modes. Execute unsupported writes directly in Spark. Replay legacy APPEND_IF_EXISTS payloads only when their targets are known to be unpartitioned and unbucketed.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d54b8aa8-151f-4ea0-81ba-fcef6249dac9

📥 Commits

Reviewing files that changed from the base of the PR and between 4d21734 and 83bf5d4.

📒 Files selected for processing (4)

• spark/src/main/scala/io/substrait/spark/FileHolder.scala
• spark/src/main/scala/io/substrait/spark/logical/ToLogicalPlan.scala
• spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala
• spark/src/test/scala/io/substrait/spark/FileWriteSuite.scala

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Filesystem write conversion now supports unpartitioned, unbucketed append writes. Export and import paths reject unsupported write modes or settings. Tests cover successful appends and rejected cases.

Changes

Filesystem append writes

Layer / File(s)	Summary
Export append writes spark/src/main/scala/io/substrait/spark/FileHolder.scala, spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala, spark/src/test/scala/io/substrait/spark/FileWriteSuite.scala	Filesystem writes require append mode and no partition or bucket settings. Exported writes use CreateMode.UNSPECIFIED. Tests cover round-trip append execution and rejected save modes or settings.
Import append writes spark/src/main/scala/io/substrait/spark/logical/ToLogicalPlan.scala, spark/src/test/scala/io/substrait/spark/FileWriteSuite.scala	Import accepts INSERT with UNSPECIFIED or APPEND_IF_EXISTS. Tests cover legacy append execution and rejection of other create modes and UPDATE.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: nielspardon

Merge Risk: ⚪ Minimal · up to 83bf5

This change intentionally restricts Spark filesystem write conversion to unpartitioned, unbucketed appends, so unsupported writes now fail instead of silently changing semantics. No concrete merge-blocking risk is evident beyond the documented breaking change.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 83bf5

The change reduces destructive write exposure by rejecting operations and layouts that cannot be preserved. Supported append writes remain compatible. No introduced security concern was established, but caller authorization for output paths and interrupted-write recovery remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — If untrusted callers can submit executable FileHolder payloads, target exposure depends on the supplied path and permissions available to Spark execution. This path-selection capability predates the PR; caller privileges and tenant, asset, or datastore scope are not established by the inspected converter.

Trust Boundaries and Controls

• observed — The new controls validate representable write semantics: unsupported export settings fail before child serialization, and unsupported import operations or modes fail before child conversion. They do not authenticate payload provenance or authorize the output path.

Resilience and Maintainability Implications

• inferred — The converter introduces no reservation, transaction, deduplication, or recovery protocol. Atomicity, repeated-execution behavior, concurrency, cleanup, and interruption handling remain dependent on Spark execution. The synthetic catalog location also predates the PR; its runtime interaction with outputPath was not verified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise, uses Conventional Commit format, and describes the main change: rejecting lossy write conversions.
Description check	✅ Passed	The description explains the rationale, identifies the supported and rejected write cases, and includes a BREAKING CHANGE footer.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nielspardon]

Please reword "Substrait v0.102.0 restricts create_mode to CTAS" in the description. The spec defines create_mode for CTAS and says nothing about INSERT, and v0.102.0 didn't change that, so rejecting it here is this binding's compatibility choice rather than a spec rule. The inline comments cover two gaps the guards still leave open.

[bvolpato]

Thanks for the wording and scope feedback. I drafted the description to distinguish the spec’s CTAS create modes from this binding’s append-only file INSERT support, narrow the legacy-payload claim, and leave CTAS-based support for other save modes as a separate change. The local patch also covers Hive layout guards and MODIFIED_RECORDS output. The full build and independent review pass; code and description updates remain unpublished.

[bvolpato]

Hive and file writes now share the layout guards, and file writes reject MODIFIED_RECORDS before converting the input. The append path and tests are simplified. The description narrows legacy compatibility to known unpartitioned, unbucketed targets and leaves CTAS support for other save modes as separate work.

[nielspardon]

LGTM