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.

Sign up for GitHub

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

Jump to bottom

Fix marshal tinyint #265

Merged
merged 2 commits into from
Oct 1, 2024
Merged

Fix marshal tinyint #265

merged 2 commits into from
Oct 1, 2024

Conversation

illia-li
Copy link

@illia-li illia-li commented Sep 17, 2024 •
edited
Loading

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
dkropachev
dkropachev requested changes Sep 23, 2024
View reviewed changes
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 requested a review from dkropachev September 25, 2024 14:23
@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 illia-li force-pushed the il/fix/marshal/tinyint branch from d1ab786 to fbe2773 Compare September 26, 2024 18:00
@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

dkropachev
dkropachev requested changes Sep 27, 2024
View reviewed changes
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 illia-li force-pushed the il/fix/marshal/tinyint branch from fbe2773 to 26ca21d Compare September 27, 2024 20:16
@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 illia-li force-pushed the il/fix/marshal/tinyint branch from 26ca21d to 4c0dc70 Compare September 27, 2024 20:38
@illia-li illia-li requested a review from dkropachev September 27, 2024 20:40
@illia-li
Copy link
Author

Something wrong with build environment.

@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch from 4c0dc70 to e18a6a9 Compare September 28, 2024 01:15
dkropachev
dkropachev requested changes Sep 28, 2024
View reviewed changes
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
dkropachev
dkropachev reviewed Sep 28, 2024
View reviewed changes
marshal.go Outdated Show resolved Hide resolved
@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch from e18a6a9 to 1f3617c Compare September 28, 2024 13:56
@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.

dkropachev
dkropachev requested changes Sep 28, 2024
View reviewed 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 requested a review from dkropachev 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
dkropachev
dkropachev requested changes Sep 30, 2024
View reviewed changes
marshal.go Outdated Show resolved Hide resolved
dkropachev
dkropachev previously approved these changes Sep 30, 2024
View reviewed changes
@dkropachev
Copy link
Collaborator

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

@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch 2 times, most recently from a23cde3 to e0c0da5 Compare September 30, 2024 20:33
sylwiaszunejko
sylwiaszunejko previously approved these changes Oct 1, 2024
View reviewed changes
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 illia-li force-pushed the il/fix/marshal/tinyint branch from e0c0da5 to 1d0b7bd Compare October 1, 2024 12:43
@illia-li illia-li force-pushed the il/fix/marshal/tinyint branch from 1d0b7bd to 89a03ac Compare October 1, 2024 12:43
@illia-li
Copy link
Author

illia-li commented Oct 1, 2024

@illia-li , could you please rebase

Done

@illia-li illia-li requested a review from dkropachev October 1, 2024 14:24
@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
Reviewers

@dkropachev dkropachev dkropachev approved these changes

@sylwiaszunejko sylwiaszunejko sylwiaszunejko left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@illia-li @sylwiaszunejko @dkropachev