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

Create utility file for encoding #8650

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Joe-Abraham
Copy link
Contributor

@Joe-Abraham Joe-Abraham commented Feb 2, 2024

Add a new utility file that shares the common logic for decoding and encoding based on the base variants.

Copy link

netlify bot commented Feb 2, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit c175d13
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6711ec2bcb467e00085e3d74

Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @Joe-Abraham.

Can you write some simple unit tests for this code ? Would really help keep the code quality as well as understand your API.

@Joe-Abraham
Copy link
Contributor Author

@aditi-pandit, I have added the test cases as you suggested. Please have a look.

velox/functions/prestosql/tests/BinaryFunctionsTest.cpp Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/functions/prestosql/tests/BinaryFunctionsTest.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@czentgr czentgr left a comment

Choose a reason for hiding this comment

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

Thanks Abe. I have a few comments.

velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
velox/common/encode/Base64.cpp Outdated Show resolved Hide resolved
velox/common/encode/tests/EncoderUtilsTests.cpp Outdated Show resolved Hide resolved
velox/common/encode/EncoderUtils.h Outdated Show resolved Hide resolved
@Yuhta
Copy link
Contributor

Yuhta commented Oct 3, 2024

Actually why not put this inside Base64.h in the same directly? The name EncoderUtils is also very ambiguous.

@Joe-Abraham
Copy link
Contributor Author

@Yuhta, The intention is to use EncoderUtils for Base32 encoding and decoding. This will ensure consistency in the logic of these functions and will remove duplicate code.

@Yuhta
Copy link
Contributor

Yuhta commented Oct 16, 2024

@Joe-Abraham Base32 is just variant of Base64 so we should put them in the same utility class.

@Joe-Abraham
Copy link
Contributor Author

@Yuhta,
This PR has entire changes that I intend to bring in. I have raised multiple PRs to ease up the review process.

It would be great if you could review this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add from_base32 Presto function
5 participants