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

Bugfix #2 - seq and map support items with first byte of 0x01. #9

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

finnbear
Copy link
Contributor

Fixes #2 - serialize a 0x00 byte before each item such that items that start with 0x01 don't confuse the deserializer.

This would affect previously-serialized keys.

Draft until #7 is merged and I can write a test case.

@finnbear
Copy link
Contributor Author

finnbear commented Apr 23, 2023

FWIW, this would be a breaking change for SurrealDB without (as far as I can tell) any benefit (the enum discriminant of Value within Id::Array is serialized as 4 big endian bytes so the first byte is always 0x00 and never 0x01).

It might make more sense to not fix this issue and simply warn other users.

@freopen
Copy link

freopen commented Apr 24, 2023

If you decide to go with a breaking change - maybe consider swapping the usage of 0x00 and 0x01 so (a, b, c) sequence is less than (a, b, c, d) sequence.

@freopen
Copy link

freopen commented Apr 24, 2023

Another idea: you can add 0x00 only if the first byte of an item is 0x01. That won't break SurrealDB, but still fix the issue.

In that case you can use 0x02 instead of 0x00 to also resolve abc < abcd problem.

@freopen
Copy link

freopen commented Apr 24, 2023

Another idea: you can add 0x00 only if the first byte of an item is 0x01. That won't break SurrealDB, but still fix the issue.

In that case you can use 0x02 instead of 0x00 to also resolve abc < abcd problem.

Oops, that won't work for items starting with 0x00, sorry

epsio-banay added a commit to Epsio-Labs/storekey that referenced this pull request Apr 9, 2024
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.

Seq serialization is ambiguous
2 participants