feat: Adds support for 0x, X'...', x'...' type hex strings in udf:encode #6118

cprasad1 · 2020-08-27T20:24:24Z

Description

Earlier, the encode udf would not recognize hex strings of types belong to {0x, X'...', x'...'} while converting to ascii, utf8 and base 64 and would give nulls. Now it recognizes them and converts them accordingly from hex to other formats. Behavior is similar to https://dev.mysql.com/doc/refman/8.0/en/hexadecimal-literals.html

Testing done

Unit + query-validation-tests + manual

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

ghost · 2020-08-27T20:24:27Z

It looks like @cprasad1 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

cprasad1 · 2020-08-27T20:44:50Z

[clabot:check]

ghost · 2020-08-27T20:44:51Z

@confluentinc It looks like @cprasad1 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

vpapavas · 2020-08-28T00:57:13Z

ksqldb-functional-tests/src/test/resources/test-runner/correct/from_docs/input.json

@@ -1,3 +1,4 @@
+


I think this is an accidental change

Can you remove this change from the PR please?

vpapavas

Great job @cprasad1 ! Congratz on your first PR!
Before merging we need to make sure that both all checks are green. We are still waiting for jenkins but there seems to be something wrong with the PR title as well (though it's not obvious to me what).

vpapavas · 2020-08-28T17:48:31Z

ksqldb-engine/src/test/java/io/confluent/ksql/function/udf/string/EncodeTest.java

@@ -41,6 +41,15 @@ public void shouldEncodeHexToAscii() {
    assertThat(udf.encode("31202b2031203d2031", "hex", "ascii"), is("1 + 1 = 1"));
    assertThat(udf.encode("ce95cebbcebbceacceb4ceb1", "hex", "ascii"), is("������������"));
    assertThat(udf.encode("c39c6265726d656e736368", "hex", "ascii"), is("��bermensch"));
+


What happens if the string is 0x0x? Or 0X? Or X' without a closing quote? Are there any characters that are not allowed with these formats?

I am assuming that you are worried about illegal hex literals (eg: 'p', 'x', '*' etc) in these formats. The function would throw an exception and we would get nulls in the end which was the default behavior before this was implemented. That is because this feature piggybacks on the pre-existing hex decoders that existed before these changes. I am adding a few more tests to demonstrate these corrupted formats.

Also, I noticed that the pre-existing encode udf couldn't deal with Hex of odd lengths (It would throw an exception). Now, it supports odd lengths by padding a "0" to the left in the 0x... format but would throw an exception with X'...' and x'...' formats which is consistent with https://dev.mysql.com/doc/refman/8.0/en/hexadecimal-literals.html. But I am not sure if we should support hex of odd lengths before without any of these special characters around them. It would just throw an exception like it used to

added more tests to demonstrate that behavior

AlanConfluent · 2020-08-31T18:02:20Z

Not sure if this is part of a larger set of changes, but with this UDF, you already give the hint of what the string is encoded as, e.g. encode("4578616d706C6521", 'hex', 'ascii').

I thought that supporting a literal means that anywhere that you can use a normal ASCII string today, you could instead write 0x4578616d706C6521, without the need to explicitly call encode since the hint is in the "0x." I would assume that the internal representation is UTF8.

vpapavas

LGTM

cprasad1 · 2020-09-01T18:35:11Z

Accidentally closed this. @agavra do we want to support the Hex literals via the encode() method or in general?

agavra · 2020-09-01T22:48:54Z

I think this PR is a valuable improvement to the UDF, but the ticket (#5544) specifically refers to the ability to (as @AlanConfluent thought) write 0xdeadbeef anywhere and have it be recognized as a hex literal. You can see the corresponding MySQL documentation on HEX literals (linked in the above ticket): https://dev.mysql.com/doc/refman/8.0/en/hexadecimal-literals.html

vpapavas · 2020-09-03T18:25:04Z

My bad, when I asked @agavra about the ticket he pointed me to the encode UDF, so I thought that's what he meant. I know realize he meant to use the encode udf to support hex literals. We will still merge this PR since it is still valuable and a step in the right direction but NOT close the ticket. Do you agree? @AlanConfluent , @agavra ? If yes, can you give the green light please?

agavra · 2020-09-03T18:28:25Z

I haven't reviewed the PR, but I agree that the feature is valuable! Green light from my perspective (I trust your code review @vpapavas 😉 ).

AlanConfluent · 2020-09-03T18:31:49Z

We will still merge this PR since it is still valuable and a step in the right direction but NOT close the ticket. Do you agree? @AlanConfluent , @agavra ? If yes, can you give the green light please?

Yes, that makes sense. The same logic can potentially just be applied in a different place and it's fine to have that as part of the UDF as well.

AlanConfluent

LGTM

cprasad1 · 2020-09-03T20:15:04Z

updated the PR description. Will merge as soon as checks are completed

…ode (confluentinc#6118)

adds support for 0x, X'', x'' type hex in udf:encode

2ee4f8d

cprasad1 requested a review from a team as a code owner August 27, 2020 20:24

cprasad1 marked this pull request as draft August 27, 2020 20:32

cprasad1 marked this pull request as ready for review August 27, 2020 20:38

vpapavas self-assigned this Aug 27, 2020

vpapavas reviewed Aug 28, 2020

View reviewed changes

vpapavas approved these changes Aug 28, 2020

View reviewed changes

added one line of comment

a404b0b

vpapavas reviewed Aug 28, 2020

View reviewed changes

added some more tests for odd length hex and corrupted hex

d61f269

vpapavas requested review from AlanConfluent and vpapavas August 31, 2020 17:45

remove input.json from pr

d56a975

vpapavas approved these changes Sep 1, 2020

View reviewed changes

cprasad1 closed this Sep 1, 2020

cprasad1 reopened this Sep 1, 2020

AlanConfluent approved these changes Sep 3, 2020

View reviewed changes

agavra merged commit d492556 into confluentinc:master Sep 3, 2020

sarwarbhuiyan pushed a commit to sarwarbhuiyan/ksql that referenced this pull request Sep 4, 2020

feat: Adds support for 0x, X'...', x'...' type hex strings in udf:enc…

e48de36

…ode (confluentinc#6118)

cprasad1 deleted the Support_HEX_Literals branch September 16, 2020 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds support for 0x, X'...', x'...' type hex strings in udf:encode #6118

feat: Adds support for 0x, X'...', x'...' type hex strings in udf:encode #6118

cprasad1 commented Aug 27, 2020 •

edited

Loading

ghost commented Aug 27, 2020

cprasad1 commented Aug 27, 2020

ghost commented Aug 27, 2020

vpapavas Aug 28, 2020

vpapavas Aug 31, 2020

cprasad1 Aug 31, 2020

vpapavas left a comment

vpapavas Aug 28, 2020

cprasad1 Aug 28, 2020

cprasad1 Aug 28, 2020 •

edited

Loading

cprasad1 Aug 28, 2020

AlanConfluent commented Aug 31, 2020

vpapavas left a comment

cprasad1 commented Sep 1, 2020

agavra commented Sep 1, 2020

vpapavas commented Sep 3, 2020 •

edited

Loading

agavra commented Sep 3, 2020

AlanConfluent commented Sep 3, 2020

AlanConfluent left a comment

cprasad1 commented Sep 3, 2020

feat: Adds support for 0x, X'...', x'...' type hex strings in udf:encode #6118

feat: Adds support for 0x, X'...', x'...' type hex strings in udf:encode #6118

Conversation

cprasad1 commented Aug 27, 2020 • edited Loading

Description

Testing done

Reviewer checklist

ghost commented Aug 27, 2020

cprasad1 commented Aug 27, 2020

ghost commented Aug 27, 2020

vpapavas Aug 28, 2020

Choose a reason for hiding this comment

vpapavas Aug 31, 2020

Choose a reason for hiding this comment

cprasad1 Aug 31, 2020

Choose a reason for hiding this comment

vpapavas left a comment

Choose a reason for hiding this comment

vpapavas Aug 28, 2020

Choose a reason for hiding this comment

cprasad1 Aug 28, 2020

Choose a reason for hiding this comment

cprasad1 Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

cprasad1 Aug 28, 2020

Choose a reason for hiding this comment

AlanConfluent commented Aug 31, 2020

vpapavas left a comment

Choose a reason for hiding this comment

cprasad1 commented Sep 1, 2020

agavra commented Sep 1, 2020

vpapavas commented Sep 3, 2020 • edited Loading

agavra commented Sep 3, 2020

AlanConfluent commented Sep 3, 2020

AlanConfluent left a comment

Choose a reason for hiding this comment

cprasad1 commented Sep 3, 2020

cprasad1 commented Aug 27, 2020 •

edited

Loading

cprasad1 Aug 28, 2020 •

edited

Loading

vpapavas commented Sep 3, 2020 •

edited

Loading