adamreeve commented on code in PR #55423:
URL: https://github.com/apache/arrow/pull/55423#discussion_r4518380960
##########
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:
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.
--
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]
