Skip to content

CLI: fix binary syntax has a bug to add one backtick#352

Open
chidea wants to merge 1 commit intoskytable:nextfrom
chidea:next
Open

CLI: fix binary syntax has a bug to add one backtick#352
chidea wants to merge 1 commit intoskytable:nextfrom
chidea:next

Conversation

@chidea
Copy link
Copy Markdown

@chidea chidea commented Jun 5, 2024

How to reproduce bug

create space test
create model test.test(id:uint8, v:binary)
insert into test.test(0, ``)
select v from test.test where id=0

and the result is ([96]) where it must be ([]).

Reason

The closing backtick(ascii:96) was added into query parameter because of wrong range to take.

✔️ By submitting this pull request, I agree to the CLA at: https://cla.skytable.io/skytable/skytable

@chidea chidea requested a review from ohsayan as a code owner June 5, 2024 03:09
@chidea chidea changed the title fix : cli binary syntax has a bug to add one backtick CLI: fix binary syntax has a bug to add one backtick Jun 5, 2024
@BitHeaven-Official
Copy link
Copy Markdown

BitHeaven-Official commented Jun 5, 2024 •
edited
Loading

You need use " or ' for Values.
96 is dec val of bin ascii ` (` (ascii) -> 01100000 (bin) -> 96 (dec))

@chidea
Copy link
Copy Markdown
Author

chidea commented Jun 7, 2024

@BitHeaven-Official " or ' is for string type values and ` is for binary type values. While string types are read by Parameterizer::read_string function, binary types are read by Parameterizer::read_binary function.
cli/src/query.rs:145

ohsayan
ohsayan reviewed Jul 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ohsayan ohsayan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! Do you think you'll be able to add some tests? If not, no issues...I'll add them later :)

@glydr
Copy link
Copy Markdown
Collaborator

glydr commented Aug 1, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ohsayan
Copy link
Copy Markdown
Member

ohsayan commented Aug 7, 2024

Urgh, I completely forgot to merge this fix into this release! Grrr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ohsayan ohsayan ohsayan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chidea @BitHeaven-Official @glydr @ohsayan