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.

Sign up for GitHub

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

Jump to bottom

Feature: PPL - json_set function #3271

Open
wants to merge 97 commits into
base: main
Choose a base branch
from
Open

Feature: PPL - json_set function #3271

wants to merge 97 commits into from

Conversation

andy-k-improving
Copy link
Contributor

@andy-k-improving andy-k-improving commented Jan 28, 2025 •
edited
Loading

This PR depends on t he json_valid, json_object and json_extract:
#3262, #3242, #3230

Description

To introduce Json function json_set to perform insert || override with provided JsonPath String.

Related Issues

Resolves: #3214
Partially resolves: #3028

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

14yapkc1 and others added 30 commits January 3, 2025 10:54
@14yapkc1
added implementation
70152eb 
Signed-off-by: Kenrick Yap <[email protected]>
@14yapkc1
added doctest, integ-tests, and unit tests
76c3995 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
addressed PR comments
ce2c551 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
fixed unit tests
ad1bde3 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
addressed pr comments
ccf47a2 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
addressed PR comments
acc76a0 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
removed unused dependencies
519c6f2 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
linting
2e319fe 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
addressed pr comment and rolling back disabled test case
ee0820d 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
Merge branch 'main' into feature/json-valid
d44fc5a 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
removed disabled import
3407d4a 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap @acarbonetto
Update docs/user/ppl/functions/json.rst
7ef6cc9 
Co-authored-by: Andrew Carbonetto <[email protected]>
Signed-off-by: kenrickyap <[email protected]>
@kenrickyap @acarbonetto
Update integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionIT…
e5e90ac 
….java

Co-authored-by: Andrew Carbonetto <[email protected]>
Signed-off-by: kenrickyap <[email protected]>
@kenrickyap
nit
2187a5a 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
Merge branch 'feature/json-valid' of https://github.com/Bit-Quill/ope…
5e1e488 
…nsearch-project-sql into feature/json-valid

Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
fixed integ test
3512b33 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
change text type to keyword
9fea606 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
addressed PR comments
fbc54bc 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
fix doc-test
31ad2a4 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
added null test
2b2a8f3 
Signed-off-by: Kenrick Yap <[email protected]>
@acarbonetto
Merge branch 'main' into feature/json-valid
dc96563
@acarbonetto
SQL: adding error case unit tests for json_valid
1913bfe 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
json_valid: null and missing should return false
67d979d 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
PPL: Add json and cast to json functions
aa6b723 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
PPL: Update json cast for review
4c99235 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
Fix testes
9ccde7f 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
spotless
4306bf3 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
Fix tests
613137b 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
SPOTLESS
ab28872 
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto
Clean up for merge
3ec16e0 
Signed-off-by: Andrew Carbonetto <[email protected]>
currantw
currantw reviewed Jan 31, 2025
View reviewed changes
Copy link
Contributor

@currantw currantw left a comment

Choose a reason for hiding this comment

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

Reviewed to 2509fc8.

core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Outdated Show resolved Hide resolved
core/src/test/java/org/opensearch/sql/expression/json/JsonFunctionsTest.java Outdated Show resolved Hide resolved
docs/user/ppl/functions/json.rst Outdated Show resolved Hide resolved
docs/user/ppl/functions/json.rst Outdated Show resolved Hide resolved
integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionsIT.java Show resolved Hide resolved
currantw
currantw reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@currantw currantw left a comment

Choose a reason for hiding this comment

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

Reviewed to 24eec27. Sorry about all the duplicate comments! GitHub seems to do that for some reason when I comment on an existing thread as part of a review. Guess I'll just stick with single comments from here on... 😬

currantw
currantw approved these changes Feb 7, 2025
View reviewed changes
Copy link
Contributor

@currantw currantw left a comment

Choose a reason for hiding this comment

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

Reviewed to c52db67. Looks good to me. Thanks for addressing all my comments. Please feel free to address any remaining unresolved threads.

@kenrickyap
addressing PR comments
0d1cc28 
Signed-off-by: Kenrick Yap <[email protected]>
acarbonetto
acarbonetto reviewed Feb 12, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/ast/expression/Cast.java Outdated
@@ -56,6 +57,7 @@ public class Cast extends UnresolvedExpression {
.put("timestamp", CAST_TO_TIMESTAMP.getName())
.put("datetime", CAST_TO_DATETIME.getName())
.put("ip", CAST_TO_IP.getName())
.put("json", CAST_TO_JSON.getName())
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you rebase please?

core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java Show resolved Hide resolved
kenrickyap and others added 17 commits February 12, 2025 10:02
@kenrickyap
added multi path use case
b6ae5ba 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
Merge branch 'main' into feature/json-extract
adde88d 
Signed-off-by: Kenrick Yap <[email protected]>

# Conflicts:
#	core/src/main/java/org/opensearch/sql/expression/DSL.java
#	core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java
#	core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java
#	core/src/main/java/org/opensearch/sql/utils/JsonUtils.java
#	core/src/test/java/org/opensearch/sql/expression/json/JsonFunctionsTest.java
#	docs/user/ppl/functions/json.rst
#	integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionsIT.java
#	integ-test/src/test/resources/json_test.json
#	ppl/src/main/antlr/OpenSearchPPLLexer.g4
#	ppl/src/main/antlr/OpenSearchPPLParser.g4
@kenrickyap
linting
aa8b81e 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap
fixing doc tests
ec6ff5e 
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap @currantw
Update core/src/main/java/org/opensearch/sql/utils/JsonUtils.java
95e996b 
Co-authored-by: Taylor Curran <[email protected]>
Signed-off-by: kenrickyap <[email protected]>
@andy-k-improving
Update jsonSet
527415e 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Provide primitive support
aefa9cb 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Manual test
3fabe17 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
IT tests
46183d9 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
IT test
afd7a31 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Code review
6daca82 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Fix doc test
b1cb3aa 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Update supported types
741322a 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Code comments
8d26f6b 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Update the test-case
c5b4da6 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Rebase
b44d1a1 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Code style
85c23f9 
Signed-off-by: Andy Kwok <[email protected]>
Yury-Fridlyand
Yury-Fridlyand reviewed Feb 14, 2025
View reviewed changes
core/src/main/java/org/opensearch/sql/utils/JsonUtils.java
String jsonPathString = path.stringValue();
Object valueToInsertObj = valueToInsert.value();
Configuration conf =
Configuration.defaultConfiguration().addOptions(Option.SUPPRESS_EXCEPTIONS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why suppress?

docs/user/ppl/functions/json.rst
Description
>>>>>>>>>>>

Usage: `json_set(json_string, json_path, value)` Perform value insertion or override with provided Json path and value. Returns the updated JSON object if valid, null otherwise.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments I posted in #3262 are also applicable there.

docs/user/ppl/functions/json.rst

Usage: `json_set(json_string, json_path, value)` Perform value insertion or override with provided Json path and value. Returns the updated JSON object if valid, null otherwise.

Argument type: STRING, STRING, BYTE/SHORT/INTEGER/LONG/FLOAT/DOUBLE/STRING/BOOLEAN/DATE/TIME/TIMESTAMP/INTERVAL/IP/STRUCT/ARRAY
Copy link
Collaborator

Choose a reason for hiding this comment

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

This becomes unreadable. maybe something like

Suggested change
Argument type: STRING, STRING, BYTE/SHORT/INTEGER/LONG/FLOAT/DOUBLE/STRING/BOOLEAN/DATE/TIME/TIMESTAMP/INTERVAL/IP/STRUCT/ARRAY
Argument type: STRING, STRING, <any>

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

@normanj-bitquill normanj-bitquill normanj-bitquill left review comments

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand left review comments

@acarbonetto acarbonetto acarbonetto left review comments

@currantw currantw currantw approved these changes

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

At least 2 approving reviews are 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.

[FEATURE] Add json_set function to PPL [RFC] Add PPL JSON extended functions support
8 participants
@andy-k-improving @YANG-DB @normanj-bitquill @Yury-Fridlyand @acarbonetto @currantw @14yapkc1 @kenrickyap