-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add TO_BYTES/FROM_BYTES functions for bytes/string conversions #7831
Conversation
``` | ||
|
||
Converts a STRING column in the specified encoding type to a BYTES column. | ||
Supported encoding types are: 'hex', 'utf8', 'ascii', 'base64'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Supported encoding types are: 'hex', 'utf8', 'ascii', 'base64'. | |
Supported encoding types are: `hex`, `utf8`, `ascii`, `base64`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a couple of suggestions.
@@ -35,7 +35,6 @@ | |||
* with your change. Otherwise, {@link PlannedTestsUpToDateTest} fill fail if there are missing | |||
* or changed query plans. | |||
*/ | |||
@Ignore("Comment me out to regenerate the historic plans") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@Udf(description = "Converts a BYTES value to STRING in the specified encoding. " | ||
+ "The accepted encoders are 'hex', 'utf8', 'ascii', and 'base64'.") | ||
public String fromBytes(final ByteBuffer value, final String encoding) { | ||
return (value == null) ? null : BytesUtils.encode(value.array(), encoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Protobuf deserializer deserializes to a read-only byte buffer, so value.array()
might throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import static org.hamcrest.Matchers.nullValue; | ||
import static org.junit.Assert.assertThrows; | ||
|
||
public class FromBytesTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have a test for an empty byte array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
import java.nio.ByteBuffer; | ||
|
||
public class ToBytesTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have a test for an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Adds
TO_BYTES
andFROM_BYTES
functions. Supported encodings arehex
,ascii
,utf8
, andbase64
.Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist