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

refactor: Create utility file for encoding #8650

Open
wants to merge 2 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 6a06e63
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6763a9ccc1a6a000089ed072

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 2, 2024
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 Joe-Abraham force-pushed the utils branch 4 times, most recently from 5992513 to 7f503d1 Compare March 11, 2024 09:54
@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
@Joe-Abraham Joe-Abraham force-pushed the utils branch 4 times, most recently from 53c7327 to 86d7ca6 Compare March 25, 2024 08:44
@Joe-Abraham Joe-Abraham marked this pull request as ready for review March 25, 2024 13:17
@Joe-Abraham Joe-Abraham force-pushed the utils branch 3 times, most recently from 4503a3a to a3de992 Compare April 1, 2024 15:48
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
@Joe-Abraham Joe-Abraham force-pushed the utils branch 2 times, most recently from 849469f to 383ca50 Compare October 1, 2024 08:01
@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.

@Joe-Abraham Joe-Abraham force-pushed the utils branch 3 times, most recently from 438539a to c175d13 Compare October 18, 2024 05:03
@Joe-Abraham Joe-Abraham marked this pull request as ready for review October 18, 2024 05:04
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be BaseEncoderUtils. Also the functions should be put in a class like BaseEncoderUtils

@Joe-Abraham Joe-Abraham changed the title Create utility file for encoding refactor : Create utility file for encoding Dec 19, 2024
@Joe-Abraham Joe-Abraham changed the title refactor : Create utility file for encoding refactor: Create utility file for encoding Dec 19, 2024
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.

5 participants