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

Fix bigint marshal, unmarshall #291

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

illia-li
Copy link

@illia-li illia-li commented Oct 3, 2024

Fixed for bigint type:

  1. string marshal and unmarshal as nullable.
    1.1. Before: marshals "" - caused an error; unmarshals nil data into "0".
    1.2. Now: marshals and unmarshals nil data - "".
  2. custom string marshals and unmarshals, before not supported, now supported.
  3. big.Int marshals, before not supported, now supported.
  4. Marshal data, which equal math.MaxUint64, into uint64, uint, return error' before, now no error`.
  5. Unmarshall data with the length is different from 0 and 8, did not return error before, now it returns `error'.

Close issues for bigint:

  1. Marshal, unmarshal. For many cql types unmarshaling data that have bigger len than cql type, do not return an error. #246
  2. Marshal, unmarshal. For many cql types a go string is supported, but not a go custom string #243
  3. Marshal, unmarshal. cql types: tinyint, smallint, int can not be marshaled into go big.Int #244
  4. Marshal, unmarshal. Problems with zero and null data and values. #250
  5. Marshal, unmarshal. For many cql types unmarshaling data that have smaller len than cql type, do not return an error. #252

marshal.go Outdated Show resolved Hide resolved
marshal/bigint/marshal_utils.go Outdated Show resolved Hide resolved
marshal/bigint/marshal.go Outdated Show resolved Hide resolved
marshal/bigint/unmarshal.go Outdated Show resolved Hide resolved
marshal/bigint/unmarshal_utils.go Outdated Show resolved Hide resolved
marshal_5_bigint_fails_test.go Outdated Show resolved Hide resolved
marshal_5_bigint_test.go Outdated Show resolved Hide resolved
@illia-li illia-li changed the title fix bigint marshal, unmarshall Fix bigint marshal, unmarshall Oct 4, 2024
@illia-li illia-li force-pushed the il/fix/marshal/bigint branch 6 times, most recently from 9867011 to bbf22b5 Compare October 5, 2024 19:45
@illia-li
Copy link
Author

illia-li commented Oct 6, 2024

Please take a look again

sylwiaszunejko
sylwiaszunejko previously approved these changes Oct 7, 2024
Copy link
Collaborator

@sylwiaszunejko sylwiaszunejko left a comment

Choose a reason for hiding this comment

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

for me it LGTM, @dkropachev could you take a look?

marshal.go Outdated
@@ -562,7 +584,7 @@ func marshalVarint(info TypeInfo, value interface{}) ([]byte, error) {
binary.BigEndian.PutUint64(retBytes, v)
}
default:
retBytes, err = marshalBigInt(info, value)
retBytes, err = marshalCounter(info, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change it to marshalCounter ?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't see these changes.
Renamed into marshalBigIntOld.

@dkropachev dkropachev merged commit 197f95a into scylladb:master Oct 8, 2024
1 check passed
@illia-li illia-li deleted the il/fix/marshal/bigint branch October 8, 2024 13:41
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.

3 participants