feat(spark): Implement Spark functions url_encode, url_decode and try_url_decode

[anhvdq]

Which issue does this PR close?

• Part of [EPIC] Complete datafusion-spark Spark Compatible Functions #15914

Rationale for this change

What changes are included in this PR?

Implement Spark functions url_encode, url_decode and try_url_decode

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

[timsaucer]

Could we add a test case that covers this replacement?

[timsaucer]

So for encoding you don't need to do the inverse operations you do in decoding? Like the + -> ' ' manipulation? I am unfamiliar with the specific needs here.

[anhvdq]

@timsaucer Let me explain a bit about the specific needs here. What we are trying to archieve in encode/decode functions are to transform the string from/to application/x-www-form-urlencoded format. The url::form_urlencoded crate is already support this transformation.

• So for the encode function, we can just simply use the byte_serialize function exported from the crates
• For decode function, there are 2 drawbacks:
  • The crate doesn't expose decode function for public use
  • It doesn't return Error when there is invalid format, for example: %2s is an invalid percent encoded string

This is the reason why on the url_decode, I tried to do the transformation manually. It's actually the same code as the decode function in the crate except that there is a valiation step.

So to answer your question:

• encode: The function already did the necessary manipulation
• decode: The decode function in the crate is not exposed and there's no format validation, so we try to copy the logic of the decode function in the crate with a validation step

Let me know if you have any suggestion on this

[timsaucer]

I am unfamiliar with the slt files, but if there was a way to have these do all 3 types of input (utf8, utf8view, largeutf8) that would be nice.

[anhvdq]

Thank @timsaucer for your feedback.
Let me check and update accordingly

[Jefffrey]

Some more test cases such as different types of inputs (as mentioned), nulls, empty strings, etc. would be beneficial 🙂

Edit: also perhaps a few roundtrip tests?

[Jefffrey]

Still seeing a lack of test cases mentioned by previous review comments: nulls, empty strings, roundtrips, plus replacement

[anhvdq]

Thank @Jefffrey
For the plus replacement, the function is copied as it is from rust-url crate which is widely used. Do we still need to add test case for this case?

Let me know if you have any suggestions

[Jefffrey]

Thank @Jefffrey For the plus replacement, the function is copied as it is from rust-url crate which is widely used. Do we still need to add test case for this case?

Let me know if you have any suggestions

Yes, we should still have a test case for this, preferably in an SLT case. Consider what happens if someone decides to refactor that code; there is currently no test case that would prevent them breaking it, I believe?

[anhvdq]

Hi @Jefffrey
I have pushed the new changes which include:

• More test cases as mentioned to cover the plus replacement, roundtrips, etc
• Add a new try_url_decode function which is basically identical to url_decode except that it returns None instead of error when the value is invalid.

Please help review and let me know your feedback
Thanks

[Jefffrey]

CI failures seem unrelated 🤔

[anhvdq]

CI failures seem unrelated 🤔

Seem so, the error code is unfamiliar to me.

[Jefffrey]

Thanks @anhvdq & @timsaucer