pitrou commented on code in PR #55423:
URL: https://github.com/apache/arrow/pull/55423#discussion_r4513527093
##########
cpp/src/arrow/dataset/partition.cc:
##########
@@ -316,12 +320,77 @@ inline std::optional<int> NextValid(const ScalarVector&
values, int first_null)
}
Result<std::vector<std::string>> KeyValuePartitioning::FormatPartitionSegments(
- const ScalarVector& values) const {
+ const ScalarVector& values, bool escape_filename_separator) const {
std::vector<std::string>
segments(static_cast<size_t>(schema_->num_fields()));
for (int i = 0; i < schema_->num_fields(); ++i) {
if (values[i] != nullptr && values[i]->is_valid) {
- segments[i] = values[i]->ToString();
+ std::string value = values[i]->ToString();
+ // Empty segments are dropped when joining path segments, so an empty
value
+ // would not be parsed back correctly
+ if (value.empty()) {
+ return Status::Invalid("Partition value for '",
schema_->field(i)->name(),
+ "' is empty, which is not supported with ",
type_name(),
+ " partitioning");
+ }
+ switch (options_.segment_encoding) {
+ case SegmentEncoding::None: {
+ // Without encoding, it's an error to write values that are not a
valid path
+ // segment. Backslashes are rejected on all platforms as they are a
path
+ // separator on Windows. Other values that are only invalid on some
+ // platforms (eg. ending in "." on Windows) are not rejected.
+ if (value.find_first_of("/\\") != std::string::npos || value == "."
||
+ value == "..") {
+ return Status::Invalid("Partition value for '",
schema_->field(i)->name(),
+ "' is not a valid path segment: '", value,
+ "'. Use SegmentEncoding::Uri to write this
value.");
+ }
+ // The value can't contain the separator between segments, or it
would
+ // not be parsed back correctly
+ if (escape_filename_separator &&
+ value.find(kFilenamePartitionSep) != std::string::npos) {
+ return Status::Invalid("Partition value for '",
schema_->field(i)->name(),
+ "' contains the partition separator '",
+ kFilenamePartitionSep, "': '", value,
+ "'. Use SegmentEncoding::Uri to write this
value.");
+ }
+ segments[i] = std::move(value);
+ break;
+ }
+ case SegmentEncoding::Uri: {
+ std::string escaped = ::arrow::util::UriEscape(value);
+ if (escape_filename_separator &&
+ 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());
Review Comment:
Why `2 *`? Are we expecting to escape two separators on average?
##########
cpp/src/arrow/dataset/partition.h:
##########
@@ -209,8 +219,17 @@ class ARROW_DS_EXPORT KeyValuePartitioning : public
Partitioning {
/// Convert a Key to a full expression.
Result<compute::Expression> ConvertKey(const Key& key) const;
+ /// Format partition values as path segments, encoding them according to
+ /// options_.segment_encoding.
+ ///
+ /// \param[in] values the partition values for each schema field
+ /// \param[in] escape_filename_separator whether values must not contain the
+ /// filename partitioning separator ("_"). It is escaped when using
Review Comment:
The double negative reads a bit weird.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
