GH-55446: [C++] Respect URI encoding in filename and directory partitioning when writing datasets by adamreeve · Pull Request #55423 · apache/arrow

adamreeve · 2026-11-06T02:32:52Z

Rationale for this change

Make dataset writing with directory and filename partitioning able to use URI encoding, to support writing values that require encoding to round-trip correctly.

What changes are included in this PR?

When writing a dataset with DirectoryPartitioning or FilenamePartitioning, respect the segment_encoding parameter and use URI encoding if it is enabled. Note that it's enabled by default, so this changes the default behaviour. Previously the segment_encoding parameter was only used when reading.

This also adds additional validation when no encoding is used, and rejects empty string values that cannot be round-tripped correctly with these two partitioning methods.

Are these changes tested?

Yes, this includes new unit tests.

Are there any user-facing changes?

Yes. DirectoryPartitioning and FilenamePartitioning both now URI encode path segments by default. This can be disabled if desired to revert to the previous behaviour by setting the the segment_encoding parameter to "none".

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 - Description and some code
AI - Code, with close supervision

Reviewed before submission by:

Human
AI
Not reviewed

GitHub Issue: [C++] Inconsistent handling of path-segment encoding for directory and filename partitioning #55446

…dataset The segment_encoding option was already respected when reading a directory-partitioned dataset, but ignored when writing. This change now URI encodes path segments if the encoding mode is set to Uri. In addition, invalid path segments now raise an error if they are invalid.

Copilot

🟢 Approval recommended

No active review issues are blocking approval of the partition segment-encoding changes.

No open issues 🎉

What changed in this PR

Addresses GH-55446 by making C++ dataset writing respect segment_encoding for directory and filename partitioning, allowing encoded partition values to round-trip correctly.

Changes:

URI-encodes partition values by default, including filename separators and characters requiring special path handling.
Rejects empty values and unsafe unencoded segments.
Adds formatting and round-trip tests, updates Python expectations, and documents the encoding behavior.

File	Description
`python/pyarrow/tests/test_dataset.py`	Updates expected formatted paths to use URI encoding.
`python/pyarrow/_dataset.pyx`	Documents encoding during writing as well as decoding during reading.
`cpp/src/arrow/dataset/partition.h`	Documents validation rules and adds the filename-separator escaping parameter.
`cpp/src/arrow/dataset/partition.cc`	Implements encoding and validation for directory and filename partition values.
`cpp/src/arrow/dataset/partition_test.cc`	Adds encoding, validation, dataset round-trip, and Hive empty-value regression tests.

_{Review effort: Balanced}

pitrou

LGTM, thank you! Just two minor comments.

pitrou · 2026-11-09T13:56:29Z

+  /// \param[in] escape_filename_separator whether values must not contain the
+  ///   filename partitioning separator ("_"). It is escaped when using


The double negative reads a bit weird.

Yeah this does sound a bit awkward, I've reworded it in f1d8404

pitrou · 2026-11-09T13:59:29Z

+              escaped.find(kFilenamePartitionSep) != std::string::npos) {
+            const std::string encoded_separator = PercentEncode(kFilenamePartitionSep);
+            std::string with_separator;
+            with_separator.reserve(escaped.size() + 2 * encoded_separator.size());


Why 2 *? Are we expecting to escape two separators on average?

Yeah this was to try to reduce the need for re-allocation. Although it doesn't account for the size of the unencoded separator character, so you actually get space for 3 separators.

I think it might be best to just remove this line. It makes the code less readable and this isn't performance critical, these strings should be small and this is only called once per partition in each dataset batch written.

I've removed this line in a14ba14

Copilot

🟢 Approval recommended

No active review issues were identified for the changes to honor segment encoding when writing dataset partitions.

No open issues 🎉

_{Review effort: Balanced}

pitrou

Thanks a lot for fixing this @adamreeve !

github-actions Bot added the awaiting review label Nov 6, 2026

This comment was marked as low quality.

Sign in to view

github-actions Bot added the Component: C++ label Nov 6, 2026

adamreeve added 10 commits November 9, 2026 14:26

Add test cases for filename partitioning

2cd88cf

Handle values containing "_" with filename partitioning

c5145e2

Reject paths with backslash too, and with a trailing "."

ab2113f

Fix inconsistent error messages

c4c8966

Simplify handling of the filename separator

a676fbf

Allow trailing space or period

c85f7e7

Disallow empty values with directory and filename partitioning

db4e2e8

Document and test handling of values starting with "." or "_"

5ff121f

Update expected results in Python tests

749c7ef

adamreeve force-pushed the directory-partition-escape branch from 5c631cf to c0445a2 Compare November 9, 2026 01:26

github-actions Bot added the Component: Python label Nov 9, 2026

Update Python docs on dataset and filename partitioning

c3959a6

adamreeve force-pushed the directory-partition-escape branch from c0445a2 to c3959a6 Compare November 9, 2026 01:33

adamreeve changed the title ~~DRAFT: [C++] Respect URI encoding setting for filename and directory partitioning when writing datasets~~ GH-55446: [C++] Respect URI encoding setting for filename and directory partitioning when writing datasets Nov 9, 2026

adamreeve changed the title ~~GH-55446: [C++] Respect URI encoding setting for filename and directory partitioning when writing datasets~~ GH-55446: [C++] Respect URI encoding in filename and directory partitioning when writing datasets Nov 9, 2026

adamreeve marked this pull request as ready for review November 9, 2026 02:25

adamreeve requested review from AlenkaF, raulcd and rok as code owners November 9, 2026 02:25

Copilot AI balanced review requested due to automatic review settings November 9, 2026 02:25

Copilot started reviewing on behalf of adamreeve November 9, 2026 02:25

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

adamreeve requested a review from jorisvandenbossche November 9, 2026 03:16

pitrou approved these changes Nov 9, 2026

View reviewed changes

github-actions Bot added awaiting committer review and removed awaiting review labels Nov 9, 2026

adamreeve added 2 commits November 10, 2026 10:49

Remove confusing string reserve

a14ba14

Improve doc comment wording for escape_filename_separator

f1d8404

Copilot AI balanced review requested due to automatic review settings November 9, 2026 21:57

Copilot started reviewing on behalf of adamreeve November 9, 2026 21:58

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

adamreeve mentioned this pull request Nov 9, 2026

GH-34905: [C++][Python] Respect segment_encoding when writing Hive partitions #55345

Open

3 of 4 tasks

pitrou approved these changes Nov 10, 2026

View reviewed changes

pitrou added Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. backport-candidate labels Nov 10, 2026

pitrou merged commit 5f42d1f into apache:main Nov 10, 2026
76 of 99 checks passed

pitrou removed the awaiting committer review label Nov 10, 2026

pitrou mentioned this pull request Nov 10, 2026

[C++] Inconsistent handling of path-segment encoding for directory and filename partitioning #55446

Closed

adamreeve deleted the directory-partition-escape branch November 10, 2026 09:00

Navigation Menu

Repository navigation

GH-55446: [C++] Respect URI encoding in filename and directory partitioning when writing datasets - #55423

GH-55446: [C++] Respect URI encoding in filename and directory partitioning when writing datasets#55423
pitrou merged 13 commits into
apache:mainfrom
adamreeve:directory-partition-escape

adamreeve commented Nov 6, 2026 •

edited by github-actions Bot

Loading

Uh oh!

This comment was marked as low quality.

Copilot AI left a comment

Uh oh!

pitrou left a comment

Uh oh!

pitrou Nov 9, 2026

Uh oh!

adamreeve Nov 9, 2026

Uh oh!

pitrou Nov 9, 2026

Uh oh!

adamreeve Nov 9, 2026

Uh oh!

adamreeve Nov 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

pitrou left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/// \param[in] escape_filename_separator whether values must not contain the
		/// filename partitioning separator ("_"). It is escaped when using

Conversation

adamreeve commented Nov 6, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

This comment was marked as low quality.

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeve Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeve Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeve Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamreeve commented Nov 6, 2026 •

edited by github-actions Bot

Loading