Skip to content
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 int/bigint/double conversion functions from bytes #8426

Merged
merged 7 commits into from
Dec 3, 2021

Conversation

spena
Copy link
Member

@spena spena commented Nov 29, 2021

Description

Add INT_FROM_BYTES, BIGINT_FROM_BYTES, and DOUBLE_FROM_BYTES functions.

All functions use the byte order BIG_ENDIAN as default. A 2nd parameter is accepted to specify the byte order:

SELECT INT_FROM_BYTES(b) ...
SELECT INT_FROM_BYTES(b, 'BIG_ENDIAN') ...
SELECT INT_FROM_BYTES(b, 'LITTLE_ENDIAN') ...

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

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 #")

@spena spena requested review from jzaralim and a team November 29, 2021 23:45
@spena
Copy link
Member Author

spena commented Nov 29, 2021

@jzaralim Instead of the proposed int_from_bytes(b, 'little'), I used int_from_bytes(b, 'little_endian'). What do you think? It maches the current ByteOrder.LITTLE_ENDIAN and also gives a better meaning when looking at the function call.

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add docs to Scalar functions?

@UdfParameter(description = "The byte order of the number bytes representation")
final String byteOrder
) {
if (byteOrder.equalsIgnoreCase(ByteOrder.BIG_ENDIAN.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do a null check for byteOrder (other two functions as well)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

public Integer intFromBytes(
@UdfParameter(description = "The bytes value to convert.")
final ByteBuffer value,
@UdfParameter(description = "The byte order of the number bytes representation")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the names of the byte orders in the description (LITTLE_ENDIAN and BIG_ENDIAN)? (also applies to the other two functions)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@spena spena force-pushed the feat_numeric_bytes_functions branch from f116957 to 6a2dd34 Compare December 1, 2021 21:46
@spena
Copy link
Member Author

spena commented Dec 1, 2021

@JimGalasyn I updated the doc

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few copy edits.

Copy link
Contributor

@jzaralim jzaralim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit

@@ -65,6 +67,18 @@ private BytesUtils() {
Encoding.BASE64, v -> base64Decoding(v)
);

public static ByteOrder byteOrderType(final String byteOrderStr) {
if (byteOrderStr.equalsIgnoreCase(ByteOrder.BIG_ENDIAN.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If byteOrderStr is null then byteOrderStr.equalsIgnoreCase will throw an NPE

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@spena spena force-pushed the feat_numeric_bytes_functions branch 2 times, most recently from 1ce8ec9 to 56e14ce Compare December 2, 2021 21:26
@spena spena force-pushed the feat_numeric_bytes_functions branch from 56e14ce to 99277e6 Compare December 3, 2021 16:54
@spena spena merged commit da77c8a into master Dec 3, 2021
@spena spena deleted the feat_numeric_bytes_functions branch December 3, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants