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 marshal tinyint #265

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Conversation

illia-li
Copy link

@illia-li illia-li commented Sep 17, 2024

Fixed for tinyint type:

  1. string marshal and unmarshal as nullable.
    1.1. Was: marshals "" - caused an error; unmarshals nil data into "0".
    1.2. Now: marshals and unmarshals nil data - "".
  2. custom string marshals and unmarshals was not supported, now supported.
  3. big.Int marshals was not supported, now supported.
  4. Also was fixed cases when marshal and unmarshal should an error, but currently we does not have a corrupt test suite.

Close issues for tinyint:

  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

@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from 790db0f to d8d240b Compare September 17, 2024 15:41
@sylwiaszunejko
Copy link
Collaborator

CI keeps failing

@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from ced833c to f43a435 Compare September 17, 2024 19:55
marshal.go Outdated Show resolved Hide resolved
marshal/interface.go Outdated Show resolved Hide resolved
marshal/erro/error.go Outdated Show resolved Hide resolved
marshal.go Outdated Show resolved Hide resolved
@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from 3fd0269 to d1ab786 Compare September 25, 2024 14:57
@illia-li
Copy link
Author

Please don`t merge this PR before #262 and #270

@sylwiaszunejko
Copy link
Collaborator

Please clean up the commits

@illia-li
Copy link
Author

Please clean up the commits

Done

@sylwiaszunejko
Copy link
Collaborator

@illia-li I would like to have the information about what exactly was fixed in the commit message, it will be helpful for future readers to have clearly stated what was wrong before and what this commit changes to make it bette

marshal/error.go Outdated Show resolved Hide resolved
marshal/tinyint/unmarshal.go Show resolved Hide resolved
marshal/tinyint/unmarshal.go Show resolved Hide resolved
marshal/tinyint/marshal.go Show resolved Hide resolved
@illia-li
Copy link
Author

@illia-li I would like to have the information about what exactly was fixed in the commit message, it will be helpful for future readers to have clearly stated what was wrong before and what this commit changes to make it bette

Done

@illia-li
Copy link
Author

Something wrong with build environment.

marshal.go Outdated Show resolved Hide resolved
marshal.go Outdated Show resolved Hide resolved
marshal/tinyint/marshal_utils.go Outdated Show resolved Hide resolved
marshal/tinyint/marshal_utils.go Outdated Show resolved Hide resolved
marshal/tinyint/marshal.go Outdated Show resolved Hide resolved
marshal/tinyint/marshal_utils.go Outdated Show resolved Hide resolved
marshal/tinyint/unmarshal.go Show resolved Hide resolved
marshal/tinyint/unmarshal_utils.go Outdated Show resolved Hide resolved
marshal_2_tinyint_test.go Outdated Show resolved Hide resolved
marshal_2_tinyint_test.go Outdated Show resolved Hide resolved
marshal.go Outdated Show resolved Hide resolved
@illia-li
Copy link
Author

Please, lets not change the test suite in this PR .
This will lead to confusion.
We already have #278 for test suite changes.

marshal/tinyint/unmarshal.go Outdated Show resolved Hide resolved
marshal_2_tinyint_test.go Outdated Show resolved Hide resolved
@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from 5d4aae8 to 1df269c Compare September 29, 2024 20:41
@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from dc88bd1 to 0e44c4f Compare September 30, 2024 16:44
marshal.go Outdated Show resolved Hide resolved
dkropachev
dkropachev previously approved these changes Sep 30, 2024
@dkropachev
Copy link
Collaborator

@sylwiaszunejko , can you please take another look at it.

sylwiaszunejko
sylwiaszunejko previously approved these changes Oct 1, 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.

LGTM

@dkropachev
Copy link
Collaborator

@illia-li , could you please rebase

@illia-li
Copy link
Author

illia-li commented Oct 1, 2024

@illia-li , could you please rebase

Done

@dkropachev dkropachev merged commit b4755c5 into scylladb:master Oct 1, 2024
1 check passed
@illia-li illia-li deleted the il/fix/marshal/tinyint branch October 4, 2024 13:12
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