ARROW-12644: [C++][Python][R][Dataset] URL-decode path segments in partitioning

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up

[lidavidm]

Now by default, directory/hive partitioning will URL-decode potential partition values before trying to parse them, since systems like Spark apparently may URL-encode the values in some cases. Note for Hive partitioning, this applies only to the value, not to the key itself. This behavior can be toggled.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12644

[lidavidm]

Ah, and so it turns out there is indeed a very good reason to url-escape names even on local file systems: Windows doesn't like characters like :. Fun! These test cases need reworking, then…

[pitrou]

I haven't looked at the R changes here, and I only skimmed through the dataset tests. Perhaps @bkietz can validate the latter.

[pitrou]

I'm curious: is there any reason we don't utf8-validate here?

[lidavidm]

Just because we didn't before, but perhaps we should.

[pitrou]

It would seem more consistent. I don't see any reason to make a difference between the two cases.

[pitrou]

I'm afraid at some point, we'll have to deal with other encodings (e.g. hex, or some system-specific oddity). Perhaps make this an enum instead, so that it's open-ended?

[pitrou]

Just FYI, I think KeyValuePartitioningOptions options = {} can be used as a shorter spelling.

[pitrou]

Is there a reason you don't use the same spelling as below, i.e. segment.substr(name_end + 1)?

[lidavidm]

Just to avoid a copy, but this can be done as string_view(segment).substr(...) instead.

[pitrou]

Since this is a boolean option, I'd rather make it keyword-only, i.e. def __init__(..., *, bint url_decode_segments=True).

[pitrou]

Nit, but we use Uri in most of the code base (including uri.h :-)).

[lidavidm]

I added UTF-8 validation and renamed URL to URI.

[pitrou]

It seems there are CI failures on Windows.

[bkietz]

LGTM, thanks for cleaning this up