-
Notifications
You must be signed in to change notification settings - Fork 257
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(golang): meta string encoding algorithm in golang #1566
Conversation
This is great! CI has some failure, could you rebase main branch? |
Actually I don't know how to restart CI tasks |
b6a6fc9
to
2f062b6
Compare
I have rebased main branch, please review this PR. |
go/fury/meta/meta_string_encoder.go
Outdated
specialChar1: e.specialChar1, | ||
specialChar2: e.specialChar2, | ||
outputBytes: e.EncodeLowerUpperDigitSpecial(input), | ||
numChars: length, |
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.
I removed the needs for numChars/numBits in #1565 , maybe we can simplify the implement a little.
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.
okay, I'll read and implement it.
go/fury/meta/meta_string_decoder.go
Outdated
// Retrieve 5 bits every iteration from data, convert them to characters, and save them to chars | ||
// "abc" encoded as [00000] [000,01] [00010] [0, corresponding to three bytes, which are 0, 68, 0 (68 = 64 + 4) | ||
// In order, take the highest digit first, then the lower | ||
chars := make([]byte, 0) |
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.
Hi @qingoba , first of all thanks for your contribution to Fury.
Could we specify the capacity when initializing chars
to avoid the performance degradation caused by slice expansion?
2f062b6
to
8a62500
Compare
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, thanks
go/fury/meta/meta_string_test.go
Outdated
"HelloWorld__123.2024": 6, | ||
"MediaContent": 5, | ||
"Apple_banana": 5, | ||
"欲海回狂": 0, // not used |
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.
How about replacing it to "你好,世界"
Co-authored-by: Shawn Yang <chaokunyang@apache.org>
Co-authored-by: Shawn Yang <chaokunyang@apache.org>
@qingoba Ci has some failures. Could you take a look? |
go/fury/meta/meta_string_test.go
Outdated
"HelloWorld__123.2024": 6, | ||
"MediaContent": 5, | ||
"Apple_banana": 5, | ||
"你好,世界": 0, // not used |
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.
Nit: format
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.
@qingoba Could you use golang format?
go/fury/meta/meta_string_test.go
Outdated
"MediaContent": ALL_TO_LOWER_SPECIAL, | ||
"HelloWorld__123.2024": LOWER_UPPER_DIGIT_SPECIAL, | ||
"Apple_banana": FIRST_TO_LOWER_SPECIAL, | ||
"你好,世界": UTF_8, |
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.
Nit: format
What does this PR do?
This PR implements meta string encoding described in xlang serialization spec
Related issues
Does this PR introduce any user-facing change?