Skip to content

[C++][Dataset] Reject partition values that are not a single path segment - #55250

Closed
Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:dataset-partition-segment-check
Closed

Nayana-Naik73 wants to merge 1 commit into
apache:mainfrom
Nayana-Naik73:dataset-partition-segment-check

Conversation

@Nayana-Naik73

Copy link
Copy Markdown

Rationale for this change

DirectoryPartitioning and FilenamePartitioning build the write path from Scalar::ToString() of each partition value with no escaping (KeyValuePartitioning::FormatPartitionSegments), unlike HivePartitioning which URI-escapes values and prefixes them with key=. The partition values come from the data being written, so a value of .. or one containing / turns into extra path components and the dataset writer creates files outside base_dir. With pyarrow 21.0.0:

t = pa.table({"x": [1, 2], "part": ["../../escaped", "a"]})
ds.write_dataset(t, "/tmp/x/out", format="parquet",
                 partitioning=ds.partitioning(pa.schema([("part", pa.string())])))
# writes /tmp/escaped/part-0.parquet

The filename flavor does the same through the file name prefix (../../fn_part-0.parquet). With existing_data_behavior="delete_matching", a value of .. makes DatasetWriter call DeleteDirContents on base_dir/.., which deleted base_dir itself in my test before it errored out.

What changes are included in this PR?

FormatPartitionSegments now returns Status::Invalid when a formatted value is . or .. or contains /. Valid values format exactly as before. Writing such values never round-tripped anyway (GH-33507), so rejecting them is the minimal fix here; URI-escaping these flavors like Hive does would change on-disk names for many more values and seems better as a separate discussion.

Are these changes tested?

Yes. Added TestPartitioning.DirectoryPartitioningFormatInvalidSegment, which fails without the fix and passes with it. The rest of the arrow-dataset-* tests pass locally.

Are there any user-facing changes?

Writing a dataset with directory or filename partitioning now fails when a partition value is ., .. or contains /, instead of writing to an unintended location.

This PR contains a "Critical Fix". Partition values taken from the written data could make the dataset writer create, and with delete_matching delete, files outside base_dir.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR, could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

This pull request has been automatically closed because you currently have 4 open pull requests, which is more than the limit of 3.

Due to the increase in pull requests opened by AI bots, and in order to keep the review queue manageable, Apache Arrow limits contributors without repository access to at most 3 concurrently open pull requests. This helps make sure each pull request gets the attention it needs and that work in progress does not go stale.

Once one of your other open pull requests has been merged or closed, you are welcome to reopen this one.

See also:

@github-actions
github-actions Bot marked this pull request as draft October 25, 2026 17:15
@github-actions github-actions Bot closed this Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant