Spark: Fix DeleteOrphanFilesSparkAction sibling-prefix scope in compareToFileList

[wombatu-kun]

Summary

Closes #16493.

DeleteOrphanFilesSparkAction.filteredCompareToFileList() previously scoped a user-supplied compareToFileList to the action's location field using a raw files.col(FILE_PATH).startsWith(location) filter. When location lacks a trailing path separator — the production-typical shape for storage URIs like s3://bucket/table returned by Table.location() — that filter also accepts sibling paths such as s3://bucket/table-backup/.... Files in those sibling directories then entered the orphan candidate set and could be deleted.

This PR normalizes the prefix to directory form via LocationUtil.stripTrailingSlash(location) + "/" before the startsWith filter. The same + "/" shape is already used in SnapshotTableSparkAction (lines 131-132) to prevent identical sibling-prefix collisions, so this aligns the orphan-files action with that existing precedent. The fix is applied symmetrically to all three currently supported Spark version trees (v3.5, v4.0, v4.1) — their source files were byte-identical for this method, so the patch is mechanical.

The directory-listing path (listedFileDS()) is unaffected: it uses Hadoop's FileSystem.listStatus from a single root, which is inherently bounded to that directory.

---

AI Disclosure

• Model: Claude Opus 4.7
• Platform/Tool: Claude Code
• Human Oversight: fully reviewed
• Prompt Summary: Fix DeleteOrphanFilesSparkAction matching sibling prefixes when the location lacks a trailing slash.

[sungwy]

Thanks @wombatu-kun - the fix looks good to me. I added a nit regarding test coverage.

Separately, do you think it would be worth clarifying the javadoc in the DeleteOrphanFiles API? the location where to look for orphan files feels a bit loose given how much behavior is implied by that parameter. If we make the matching semantics more explicit there, I think it would be clearer for both users and maintainers.

iceberg/api/src/main/java/org/apache/iceberg/actions/DeleteOrphanFiles.java

Lines 34 to 44 in 36ef887

	public interface DeleteOrphanFiles extends Action<DeleteOrphanFiles, DeleteOrphanFiles.Result> {
	/**
	* Passes a location which should be scanned for orphan files.
	*
	* <p>If not set, the root table location will be scanned potentially removing both orphan data
	* and metadata files.
	*
	* @param location the location where to look for orphan files
	* @return this for method chaining
	*/
	DeleteOrphanFiles location(String location);

[sungwy]

nit: in addition to checking the sibling location, could we include a case where file_path == location? Since the new filtering behavior excludes exact match, I think it would be helpful to add a test to call out the behavior change.

[wombatu-kun]

Good idea — I folded the exact-match case into the same scope test (renamed to testCompareToFileListExcludesPathsOutsideLocationScope, applied across v3.5/v4.0/v4.1). Alongside the sibling directory it now adds a compareToFileList entry whose path equals location exactly and asserts it isn't reported as orphan, documenting that the trailing-separator prefix excludes exact matches as deliberate behavior. Done in 341d038.

[wombatu-kun]

Thanks for the review! Good call on the javadoc — I clarified DeleteOrphanFiles.location(String) to make the matching semantics explicit: the location is treated as a directory, so only files contained within its subtree are in scope, and paths that merely share the location's prefix (a sibling directory such as .../table-backup relative to a .../table location) or equal the location itself are excluded. I also reworded the loose @param line you quoted. Done in 341d038.

[sungwy]

Hi @wombatu-kun - thanks again for working on this PR. This looks good to merge.

Just one last thing - this PR introduces a behavior change, and hence I think we should follow the guideline here and make sure we discuss this behavior change in the dev mailing list: https://iceberg.apache.org/contribute/#merging-pull-requests

In short, I'd describe the rationale as to why we'd want this behavior change by referring to this issue @rdblue raised: #16493 . I don't think we'd need a whole proposal since this change is relatively straight forward, but a '[DISCUSS] <proposal title>' email to the dev list would allow users to voice concerns regarding the behavior change first, before we move forward with the merge.

[wombatu-kun]

Thanks @sungwy. I'd push back on the need for a separate [DISCUSS] thread here.

This is a straightforward bug fix, not an intentional behavior change — the startsWith(location) filter matched sibling prefixes only because it was missing the trailing path separator.

More to the point, both the problem and the fix direction were already settled on the issue itself — it was raised by @rdblue and triaged by @RussellSpitzer as a normal bug worth fixing. With the project's most senior maintainers having already agreed on the rationale and the approach, and given how minimal the change is, a dev-list thread or survey would mostly duplicate that and ask the community to re-decide something that's effectively already decided.

@rdblue @RussellSpitzer — does a separate [DISCUSS] email add anything here, or does #16493 already cover it?

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

[wombatu-kun]

not stale

[wombatu-kun]

A duplicate fix for the same bug just landed as #16999, independently arriving at the same trailing-separator approach. That is further evidence this is an uncontroversial bug fix users are hitting, not an intentional behavior change.

@sungwy @rdblue @RussellSpitzer - the only open question here is whether a separate [DISCUSS] thread is needed before merge. Given the bug was already triaged on #16493 and the fix is minimal, could we move ahead? Happy to send a short dev-list note if you'd still prefer one.

[uros-b]

@wombatu-kun PTAL: #17448 (review)

[uros-b]

cc @alessandro-nori to coordinate ^^

[alessandro-nori]

the changes look good to me but we usually only target the latest Spark version in the first PR. If the changes are accepted and merged we then create another PR to update the other versions