Refactor Base64 class to match coding style #10371

Joe-Abraham · 2024-07-02T08:32:37Z

Rewrite to match the Velox coding guidelines.
Rename constCharsetContains to findCharacterInCharset for clarity.
Add comments.

netlify · 2024-07-02T08:32:57Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`2de0472`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66b4dfb3fa75ff0008b7ea8b

majetideepak

@Joe-Abraham Some high-level comments.

velox/common/encode/Base64.h

velox/common/encode/Base64.cpp

velox/common/encode/Base64.h

velox/common/encode/Base64.cpp

Joe-Abraham

Made the changes as per your comments

velox/common/encode/Base64.h

velox/common/encode/Base64.cpp

majetideepak

@Joe-Abraham Can you clarify the changes to the API?

velox/common/encode/Base64.h

Joe-Abraham

updated the comments

velox/common/encode/Base64.h

velox/common/encode/Base64.cpp

majetideepak · 2024-07-19T13:10:01Z

velox/common/encode/Base64.cpp

-constexpr bool constCharsetContains(
+
+/// Searches for a character within a charset up to a certain index.
+static constexpr bool findCharacterInCharSet(


remove constexpr

it is indirectly used by static assertion, so couldn't be removed

velox/common/encode/Base64.cpp

Joe-Abraham · 2024-07-21T07:59:54Z

@majetideepak I have made the following changes

checkForwardIndex, findCharacterInCharSetand checkReverseIndex, and other necessary functions have been moved to the anonymous namespace.
checkForwardIndex and checkReverseIndex are used in static assertion, so constexpr can't be removed from all three functions.
I have added new test cases for findCharacterInCharSet.
For the functions checkForwardIndex, findCharacterInCharSet, and checkReverseIndex, I tried moving the function definition to the cpp file, and I am running into the compiler error. I think it is impossible to keep the definition and declaration of the constexpr functions separate. I did a bit of research and found this https://stackoverflow.com/questions/27345284/is-it-possible-to-declare-constexpr-class-in-a-header-and-define-it-in-a-separat

majetideepak

@Joe-Abraham I just noticed the usage of checkForwardIndex and checkReverseIndex. They are only used by the static_assert in the cpp file.
They are not used by any other function. Can you clarify the scope of their refactor and the new tests added? Do we need to test outside of those static asserts?

majetideepak

@Joe-Abraham One comment. Thanks!

velox/common/encode/tests/Base64Test.cpp

velox/common/encode/Base64.cpp

majetideepak · 2024-08-08T15:29:10Z

@bikramSingh91, @mbasmanova do you have any feedback on this? Thanks!

mbasmanova · 2024-08-09T10:37:17Z

velox/common/encode/tests/Base64Test.cpp

@@ -86,4 +89,14 @@ TEST_F(Base64Test, calculateDecodedSizeProperSize) {
  EXPECT_EQ(14, encoded_size);
 }

+TEST_F(Base64Test, checksPadding) {


nit: consider renaming these test to match the methods they are testing: isPadded and numPadding.

mbasmanova · 2024-08-09T10:37:28Z

velox/common/encode/tests/Base64Test.cpp

@@ -59,9 +62,9 @@ TEST_F(Base64Test, calculateDecodedSizeProperSize) {
  EXPECT_EQ(18, encoded_size);

  encoded_size = 21;
-  EXPECT_THROW(
+  VELOX_ASSERT_THROW(


mbasmanova

@Joe-Abraham Thank you for the cleanup.

mbasmanova · 2024-08-09T10:38:22Z

velox/common/encode/Base64.h

  template <class T>
  static std::string
  encodeImpl(const T& data, const Charset& charset, bool include_pad);

+  /// Encodes the specified data using the provided charset.


I assume here the caller is expected to allocate 'out' of sufficient length. It would be nice to clarify this in the comment.

mbasmanova · 2024-08-09T10:39:13Z

velox/common/encode/Base64.h

-  static uint8_t Base64ReverseLookup(char p, const ReverseIndex& table);
+  /// Performs a reverse lookup in the reverse index to retrieve the original
+  /// index of a character in the base.
+  static uint8_t base64ReverseLookup(char p, const ReverseIndex& reverseIndex);


Is this a private method? If so, comments should use //

mbasmanova · 2024-08-09T10:39:40Z

velox/common/encode/Base64.h

  }

-  static inline size_t countPadding(const char* src, size_t len) {
+  /// Counts the number of padding characters in encoded data.
+  static inline size_t numPadding(const char* src, size_t len) {


nit: using std::string_view for the argument would be nicer

mbasmanova · 2024-08-09T10:39:52Z

velox/common/encode/Base64.h

 private:
+  /// Checks if there is padding in encoded data.


// for private methods

mbasmanova · 2024-08-09T10:40:52Z

velox/common/encode/Base64.h

  static size_t
  decode(const char* src, size_t src_len, char* dst, size_t dst_len);

+  /// Decodes the specified number of characters from the 'src' using URL


It would be nice to add some links to material that explains the difference between 'encoding' and 'URL encoding'.

Would you clarify what happens if 'src' cannot be decoded?

mbasmanova · 2024-08-09T10:41:30Z

velox/common/encode/Base64.h

  static void decodeUrl(
      const std::pair<const char*, int32_t>& payload,
      std::string& output);
+
+  /// Decodes the specified URL encoded text.
  static std::string decodeUrl(folly::StringPiece text);


Would be nice to replace folly::StringPiece with std::string_view

mbasmanova · 2024-08-09T10:41:55Z

velox/common/encode/Base64.h

  static std::string encodeUrl(folly::StringPiece text);
+
+  /// Decodes the specified URL encoded payload and writes the result to the
+  /// 'output'.
  static void decodeUrl(
      const std::pair<const char*, int32_t>& payload,


Can we use std::string_view here?

mbasmanova · 2024-08-09T10:42:41Z

velox/common/encode/Base64.h

+  /// Reverse lookup table for decoding purposes.
+  /// Maps each possible encoded character to its corresponding numeric value
+  /// within the encoding base.
+  using ReverseIndex = std::array<uint8_t, kReverseIndexSize>;


Does this need to be public?

mbasmanova · 2024-08-09T10:43:32Z

velox/common/encode/Base64.cpp

  if (!src_len) {
    return 0;
  }

  auto needed = calculateDecodedSize(src, src_len);
  if (dst_len < needed) {
-    throw Base64Exception(
+    VELOX_USER_FAIL(


Would be nice to make this API non-throwing: https://velox-lib.io/blog/optimize-try-more

mbasmanova · 2024-08-09T10:44:00Z

velox/common/encode/Base64.cpp

 template <class T>
-/*  static */ std::string
-Base64::encodeImpl(const T& data, const Charset& charset, bool include_pad) {
+/* static */ std::string Base64::encodeImpl(


facebook-github-bot · 2024-08-12T16:08:04Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-12T20:38:34Z

@xiaoxmeng merged this pull request in a4389f9.

conbench-facebook · 2024-08-12T21:07:21Z

Conbench analyzed the 1 benchmark run on commit a4389f93.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Joe-Abraham · 2024-10-02T16:59:37Z

@majetideepak I will add a new PR for status change alone first, and then we will return to this PR.

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 Jul 2, 2024

Joe-Abraham marked this pull request as draft July 2, 2024 08:32

Joe-Abraham force-pushed the clean_base64 branch from d77986c to af52cf4 Compare July 2, 2024 08:42

Joe-Abraham mentioned this pull request Jul 3, 2024

Add from_base32 Presto function #7294

Open

Joe-Abraham marked this pull request as ready for review July 3, 2024 05:21

Joe-Abraham mentioned this pull request Jul 3, 2024

Create utility file for encoding #8650

Draft

majetideepak reviewed Jul 3, 2024

View reviewed changes

Joe-Abraham force-pushed the clean_base64 branch from af52cf4 to dbd3e4f Compare July 3, 2024 09:14

Joe-Abraham commented Jul 3, 2024

View reviewed changes

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.cpp Outdated Show resolved Hide resolved

velox/common/encode/Base64.cpp Outdated Show resolved Hide resolved

Joe-Abraham requested a review from majetideepak July 3, 2024 09:32

Joe-Abraham force-pushed the clean_base64 branch 2 times, most recently from 72f2e1e to 7b31e43 Compare July 9, 2024 05:01

majetideepak reviewed Jul 11, 2024

View reviewed changes

velox/common/encode/Base64.h Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

Joe-Abraham commented Jul 12, 2024

View reviewed changes

velox/common/encode/Base64.h Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

Joe-Abraham force-pushed the clean_base64 branch from 7b31e43 to a28984a Compare July 15, 2024 04:41

Joe-Abraham requested a review from majetideepak July 16, 2024 06:36

majetideepak reviewed Jul 18, 2024

View reviewed changes

velox/common/encode/Base64.h Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

velox/common/encode/Base64.h Outdated Show resolved Hide resolved

Joe-Abraham force-pushed the clean_base64 branch 4 times, most recently from e659d21 to 4ade5dc Compare July 19, 2024 12:02

majetideepak reviewed Jul 19, 2024

View reviewed changes

Joe-Abraham force-pushed the clean_base64 branch 2 times, most recently from 94a53ba to 570bb3e Compare July 21, 2024 07:52

Joe-Abraham force-pushed the clean_base64 branch 3 times, most recently from ccc856e to 551b286 Compare July 22, 2024 03:31

Joe-Abraham requested a review from majetideepak July 23, 2024 03:43

majetideepak reviewed Jul 23, 2024

View reviewed changes

Joe-Abraham force-pushed the clean_base64 branch 5 times, most recently from 9780b07 to f2727c7 Compare August 8, 2024 14:24

majetideepak reviewed Aug 8, 2024

View reviewed changes

velox/common/encode/tests/Base64Test.cpp Outdated Show resolved Hide resolved

majetideepak reviewed Aug 8, 2024

View reviewed changes

velox/common/encode/Base64.cpp Outdated Show resolved Hide resolved

Rewrite base64 to match the latest coding guidelines

2de0472

Joe-Abraham force-pushed the clean_base64 branch from f2727c7 to 2de0472 Compare August 8, 2024 15:09

Joe-Abraham requested a review from majetideepak August 8, 2024 15:10

majetideepak approved these changes Aug 8, 2024

View reviewed changes

majetideepak requested review from bikramSingh91 and mbasmanova August 8, 2024 15:28

mbasmanova reviewed Aug 9, 2024

View reviewed changes

mbasmanova approved these changes Aug 9, 2024

View reviewed changes

mbasmanova changed the title ~~Improve base64 implementation~~ Refactor Base64 class to match coding style Aug 9, 2024

mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Aug 9, 2024

facebook-github-bot closed this in a4389f9 Aug 12, 2024

facebook-github-bot added the Merged label Aug 12, 2024

Joe-Abraham deleted the clean_base64 branch August 14, 2024 06:47

Joe-Abraham mentioned this pull request Sep 12, 2024

Update Base64 to match coding standard #10983

Draft

Joe-Abraham mentioned this pull request Oct 3, 2024

Update Base64 as a non-throwing API #11149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Base64 class to match coding style #10371

Refactor Base64 class to match coding style #10371

Joe-Abraham commented Jul 2, 2024 •

edited by majetideepak

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

majetideepak left a comment

Joe-Abraham left a comment

majetideepak left a comment

Joe-Abraham left a comment

majetideepak Jul 19, 2024

Joe-Abraham Jul 21, 2024

Joe-Abraham commented Jul 21, 2024

majetideepak left a comment •

edited

Loading

majetideepak left a comment

majetideepak commented Aug 8, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova left a comment

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

mbasmanova Aug 9, 2024

facebook-github-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

conbench-facebook bot commented Aug 12, 2024

Joe-Abraham commented Oct 2, 2024

Refactor Base64 class to match coding style #10371

Refactor Base64 class to match coding style #10371

Conversation

Joe-Abraham commented Jul 2, 2024 • edited by majetideepak Loading

netlify bot commented Jul 2, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

majetideepak left a comment

Choose a reason for hiding this comment

Joe-Abraham left a comment

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

Joe-Abraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joe-Abraham commented Jul 21, 2024

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 12, 2024

facebook-github-bot commented Aug 12, 2024

conbench-facebook bot commented Aug 12, 2024

Joe-Abraham commented Oct 2, 2024

Joe-Abraham commented Jul 2, 2024 •

edited by majetideepak

Loading

netlify bot commented Jul 2, 2024 •

edited

Loading

majetideepak left a comment •

edited

Loading