Repository navigation
[C++][Dataset] Reject partition values that are not a single path segment - #55250
Nayana-Naik73 wants to merge 1 commit into
Conversation
|
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? or After updating the title, you can mark the pull request as ready for review. See also: |
|
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: |
Rationale for this change
DirectoryPartitioningandFilenamePartitioningbuild the write path fromScalar::ToString()of each partition value with no escaping (KeyValuePartitioning::FormatPartitionSegments), unlikeHivePartitioningwhich URI-escapes values and prefixes them withkey=. 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 outsidebase_dir. With pyarrow 21.0.0:The filename flavor does the same through the file name prefix (
../../fn_part-0.parquet). Withexisting_data_behavior="delete_matching", a value of..makesDatasetWritercallDeleteDirContentsonbase_dir/.., which deletedbase_diritself in my test before it errored out.What changes are included in this PR?
FormatPartitionSegmentsnow returnsStatus::Invalidwhen 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 thearrow-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_matchingdelete, files outsidebase_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:
Reviewed before submission by: