Skip to content

[core] Keep staging trees out of an overwrite that clears a prefix - #8905

Open
sundapeng wants to merge 2 commits into
apache:masterfrom
sundapeng:fix/format-table-overwrite-staging-trees
Open

[core] Keep staging trees out of an overwrite that clears a prefix#8905
sundapeng wants to merge 2 commits into
apache:masterfrom
sundapeng:fix/format-table-overwrite-staging-trees

Conversation

@sundapeng

Copy link
Copy Markdown
Member

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 OVERWRITE lists the partition it clears with fileIO.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 by year, month clears year=2025, and a job writing the same prefix with a dynamic month stages under it:

year=2025/_temporary/attempt/part.csv
year=2025/_temporary/0/attempt_x/month=12/part-00012.csv

Both go, because every intermediate directory name looks ordinary.

Fix

Delete what listDataFiles returns, 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/_temporary holds 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 the key=value layout a partition directory is always month=..., so nothing hidden is descended at all.

FormatTableScan.isDataFileName has 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 dynamic month directories 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 _temporary next 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.
mutation result
descend any hidden directory at a partition level 2 tests fail
pass depth 0 instead of partitionKeys - staticPartitions 1 test fails

135 tests pass across FormatTableScanTest, FormatTableCommitTest, CatalogManagedPartitionScanTest, PartitionPathUtilsTest, FormatTableWriteTest, FormatReadBuilderTest, FormatTableCompatibilityTest and FormatDataSplitTest; spotless and checkstyle clean.

Note on _temporary

testOverwriteKeepsFilesOfConcurrentWritersStagingTrees covers the magic-committer tree only. A _temporary in the same partition is emptied by this writer's own RenamingTwoPhaseOutputStream.clean(), which still removes that shared directory recursively; #8900 fixes that separately. Once it lands, the _temporary shape belongs in this test too.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant