[core] Keep staging trees out of an overwrite that clears a prefix - #8905
Open
sundapeng wants to merge 2 commits into
Open
[core] Keep staging trees out of an overwrite that clears a prefix#8905sundapeng wants to merge 2 commits into
sundapeng wants to merge 2 commits into
Conversation
INSERT OVERWRITE listed the partition it was clearing recursively and decided what to delete from the leaf file name alone, so a file another job had staged there went with it: staged files carry ordinary data file names, and only the directory above them says they are uncommitted. That is the same confusion the read path stopped making, on the destructive side. Delete what listDataFiles returns instead, and give it the depth the static partition left open. A static partition may name only the leading keys, in which case the path is a prefix and the partition directories of the remaining keys sit below it. Being at a partition level does not exempt a directory from the '_'/'.' rule. A job writing the same prefix with the trailing keys dynamic stages exactly there, so year=2025/_temporary holds that job's own month directories, not this table's. One hidden name is table content: the default partition name in the value-only layout, where a partition directory is the bare value, which is the exemption partition discovery already makes. In the key=value layout a partition directory is always month=..., so nothing hidden is descended at all.
Deciding what is data from the file name alone is what both the read and the overwrite path stopped doing; this helper was the last trace of it and nothing in any module calls it now. Its three name-convention tests move next to the rule they are about, PartitionPathUtils.isHiddenName, and gain the null and empty cases that were only implied before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8861, which did this for the read path. The overwrite path still decides what to delete from the leaf file name alone.
Problem
INSERT OVERWRITElists the partition it clears withfileIO.listFiles(path, true)and deletes every entry whose name is not'_'/'.'-prefixed. Files another job staged there carry ordinary data file names, so they are deleted and that job then fails to commit its own output. Only the directory above them says they are uncommitted, which is exactly what the read path started using.The prefix case is worse.
INSERT OVERWRITE ... PARTITION (year = '2025')on a table partitioned byyear, monthclearsyear=2025, and a job writing the same prefix with a dynamic month stages under it:Both go, because every intermediate directory name looks ordinary.
Fix
Delete what
listDataFilesreturns, and give it the depth the static partition left open — a static partition may name only the leading keys, so the partition directories of the remaining keys sit below the path being cleared.Being at a partition level does not exempt a directory from the
'_'/'.'rule.year=2025/_temporaryholds the other job's own month directories, not this table's. One hidden name is table content: the default partition name in the value-only layout, where a partition directory is the bare value — the exemption partition discovery already makes. In thekey=valuelayout a partition directory is alwaysmonth=..., so nothing hidden is descended at all.FormatTableScan.isDataFileNamehas no caller left after this and is removed in the second commit; its three name-convention tests move next to the rule they are about,PartitionPathUtils.isHiddenName.Tests
testOverwritingAPrefixKeepsStagingTreesSittingAtAPartitionLevel— a staging root standing where the dynamicmonthdirectories do, in three shapes, while the stale committed file still goes.testOverwritingAPrefixClearsTheDefaultPartitionDirectory— the value-only default partition is cleared although its name starts with_, while a_temporarynext to it survives. That pair is what separates "exempt this one name" from "exempt anything hidden here".testOverwriteKeepsFilesOfConcurrentWritersStagingTrees— a magic-committer tree inside a fully specified partition.partitionKeys - staticPartitions135 tests pass across
FormatTableScanTest,FormatTableCommitTest,CatalogManagedPartitionScanTest,PartitionPathUtilsTest,FormatTableWriteTest,FormatReadBuilderTest,FormatTableCompatibilityTestandFormatDataSplitTest; spotless and checkstyle clean.Note on
_temporarytestOverwriteKeepsFilesOfConcurrentWritersStagingTreescovers the magic-committer tree only. A_temporaryin the same partition is emptied by this writer's ownRenamingTwoPhaseOutputStream.clean(), which still removes that shared directory recursively; #8900 fixes that separately. Once it lands, the_temporaryshape belongs in this test too.