Skip to content

support JSON_MERGE() and JSON_MERGE_PATCH()#2368

Merged
jycor merged 9 commits intomainfrom
james/merge
Mar 6, 2024
Merged

support JSON_MERGE() and JSON_MERGE_PATCH()#2368
jycor merged 9 commits intomainfrom
james/merge

Conversation

@jycor
Copy link
Copy Markdown
Contributor

@jycor jycor commented Mar 5, 2024

@jycor jycor changed the title support JSON_MERGE() syonym support JSON_MERGE() and JSON_MERGE_PATCH() Mar 6, 2024
Copy link
Copy Markdown
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 great! Just one additional test case for null behavior that I think we should cover.

@@ -0,0 +1,142 @@
// Copyright 2022 Dolthub, Inc.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2022 Dolthub, Inc.
// Copyright 2024 Dolthub, Inc.

@@ -1,4 +1,4 @@
// Copyright 2022 Dolthub, Inc.
// Copyright 2022=2024 Dolthub, Inc.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2022=2024 Dolthub, Inc.
// Copyright 2022-2024 Dolthub, Inc.

"github.com/dolthub/go-mysql-server/sql/types"
)

func TestJSONMergePatch(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The only test case I noticed missing was an object with a key with a null value. Would be good to test the key deletion behavior for json_merge_patch and probably also worth added a test for json_merge_preserve just to assert that keys with null values aren't deleted there.

@jycor jycor merged commit 52a81be into main Mar 6, 2024
@jycor jycor deleted the james/merge branch March 6, 2024 19:43
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.

2 participants