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

Add "dolt_optimize_json" system variable. #8722

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

nicktobey
Copy link
Contributor

@nicktobey nicktobey commented Jan 8, 2025

When set to 0, Dolt will write Json documents to storage as simple blobs instead of path-indexed trees.

This is useful as a workaround to a current issue where the JSON chunker won't chunk in the middle of large string literals, resulting in larger-than-expected chunks.

@nicktobey nicktobey changed the title Add "dolt_dont_optimize_json" system variable. Add "dolt_optimize_json" system variable. Jan 8, 2025
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.

Code looks good; I suggested two small updates from the sys var name change.

Looks like the new BATS test is failing in Lambda for some reason though.

Comment on lines 265 to 267
# This test inserts a large JSON document with the `dolt_dont_optimize_json` flag set.
# We expect that the document gets stored as a blob.
@test "json: Test dolt_dont_optimize_json system variable" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# This test inserts a large JSON document with the `dolt_dont_optimize_json` flag set.
# We expect that the document gets stored as a blob.
@test "json: Test dolt_dont_optimize_json system variable" {
# This test inserts a large JSON document with the `dolt_optimize_json` flag disabled.
# We expect that the document gets stored as a blob.
@test "json: Test dolt_optimize_json system variable disabled" {

Comment on lines 313 to 317
dontOptimizeJson, err := sqlCtx.Session.GetSessionVariable(sqlCtx, "dolt_optimize_json")
if err != nil {
return hash.Hash{}, err
}
if dontOptimizeJson == uint8(0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dontOptimizeJson, err := sqlCtx.Session.GetSessionVariable(sqlCtx, "dolt_optimize_json")
if err != nil {
return hash.Hash{}, err
}
if dontOptimizeJson == uint8(0) {
optimizeJson, err := sqlCtx.Session.GetSessionVariable(sqlCtx, "dolt_optimize_json")
if err != nil {
return hash.Hash{}, err
}
if optimizeJson == uint8(0) {

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
480150c ok 5937457
version total_tests
480150c 5937457
correctness_percentage
100.0

@dolthub dolthub deleted a comment from coffeegoddd Jan 8, 2025
@nicktobey nicktobey merged commit b7b2427 into main Jan 8, 2025
19 checks passed
@nicktobey nicktobey deleted the nicktobey/dont-optimize-json branch January 8, 2025 04:47
@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
4b98019 ok 5937457
version total_tests
4b98019 5937457
correctness_percentage
100.0

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