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

Expand ItemAccess::itemWidth to 32 bits #8831

Merged
merged 3 commits into from
Feb 7, 2025
Merged

Expand ItemAccess::itemWidth to 32 bits #8831

merged 3 commits into from
Feb 7, 2025

Conversation

nicktobey
Copy link
Contributor

ItemAccess is a class used to read data out of prolly tree nodes. Because the itemWidth field was limited to 16 bits, reading any value larger than 2^16 bytes would result in silent truncation.

We don't usually store values this large, although it should be safe to do so.

This issue was discovered because the new JSON chunker (introduced in #7912) always stores embedded strings as a single chunk, so a document containing a string larger than 32KB would result in a node with a single value whose length didn't fit in 16 bits.

While we were investigating this issue, we created #8723 to disable the new JSON chunker in the presence of these long strings. This PR partially reverts that one, resuming the smart chunking of JSON even in the presence of large embedded strings.

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
34f9158 ok 5937457
version total_tests
34f9158 5937457
correctness_percentage
100.0

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! Good job finding this.

integration-tests/bats/json.bats Show resolved Hide resolved
@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
c6d7f65 ok 5937457
version total_tests
c6d7f65 5937457
correctness_percentage
100.0

@nicktobey nicktobey merged commit f703df9 into main Feb 7, 2025
21 checks passed
@nicktobey nicktobey deleted the nicktobey/jsonread branch February 7, 2025 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants