Feature: PPL - json_set function #3271

andy-k-improving · 2025-01-28T18:54:12Z

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.

Signed-off-by: Kenrick Yap <[email protected]>

Co-authored-by: Andrew Carbonetto <[email protected]> Signed-off-by: kenrickyap <[email protected]>

….java Co-authored-by: Andrew Carbonetto <[email protected]> Signed-off-by: kenrickyap <[email protected]>

Signed-off-by: Kenrick Yap <[email protected]>

…nsearch-project-sql into feature/json-valid Signed-off-by: Kenrick Yap <[email protected]>

Signed-off-by: Kenrick Yap <[email protected]>

Signed-off-by: Andrew Carbonetto <[email protected]>

YANG-DB · 2025-01-28T19:03:44Z

Hi @andy-k-improving
is this still in draft mode ? if not can u plz resolve the conflicts ?
thanks

andy-k-improving · 2025-01-28T19:23:38Z

Hi @andy-k-improving is this still in draft mode ? if not can u plz resolve the conflicts ? thanks

Functionality wide, this is ready.
however my code is based on some other json function as we share some util and Gradle deps.
#3262, #3242, #3230
Hence the conflict can only be fixed after above being merged I believe.

async-query-core/src/main/antlr/OpenSearchPPLLexer.g4

async-query-core/src/main/antlr/OpenSearchPPLParser.g4

core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

docs/user/ppl/functions/json.rst

Signed-off-by: Andy Kwok <[email protected]>

andy-k-improving · 2025-01-31T19:04:49Z

@normanj-bitquill Fixed above comments, would you mind to have another look?

currantw

Reviewed to 2509fc8.

currantw · 2025-01-31T22:06:12Z

core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java

+        BuiltinFunctionName.JSON_SET.getName(),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, STRING),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, LONG),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, SHORT),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, INTEGER),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, DOUBLE),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, BOOLEAN),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, ARRAY),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, STRUCT),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, BYTE),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, DATE),
+        impl(nullMissingHandling(JsonUtils::setJson), UNDEFINED, STRING, STRING, IP));


A few questions/thoughts:

Looks like you're missing FLOAT?

You're supporting DATE. What about TIME, TIMESTAMP, or INTERVAL types?

What about UNDEFINED? Is there anyway to set a JSON value to null?

currantw · 2025-01-31T22:10:20Z

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+    String jsonUnquoted = StringUtils.unquoteText(json.stringValue());
+    String pathUnquoted = StringUtils.unquoteText(path.stringValue());


What does this method need to use unquoteText but the other JSON methods above don't?

currantw · 2025-01-31T22:17:38Z

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+    Configuration conf =
+        Configuration.defaultConfiguration().addOptions(Option.SUPPRESS_EXCEPTIONS);
+    try {
+      JsonPath jsonPath = JsonPath.compile(pathUnquoted);
+      DocumentContext docContext = JsonPath.using(conf).parse(jsonUnquoted);
+      switch (docContext.read(jsonPath)) {
+        case null -> {
+          // Insert a new property
+          recursiveCreate(docContext, pathUnquoted, valueUnquoted);
+          return new ExprStringValue(docContext.jsonString());
+        }


Why are you using SUPPRESS_EXCEPTIONS here?

From the documentation:

Suppress all exceptions when evaluating path ... If an exception is thrown and the option ALWAYS_RETURN_LIST is not present null is returned.

Won't this mean that the first switch case (insert a new property) will be executed whenever an invalid path argument is given (i.e. if it isn't even valid JSON path syntax)? Why are we handling and throwing an exception if the path isn't a valid JSON path?

@kenrickyap current PR for json_expand handles InvalidPathException. Probably make sense for these two functions (and all other JSON functions) to handle this the same? Would it be possible to extract this logic to a shared method?

private static JsonPath getJsonPath(String jsonPathString) { try { return JsonPath.compile(jsonPathString); } catch () { final String errorMsgFormat = "JSON path '%s' is not valid. Error details: %s"; throw new SemanticCheckException(String.format(errorMsgFormat, jsonPathString, e.getMessage()), e); } }

currantw · 2025-01-31T22:25:46Z

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+  }
+
+  /** Converts a JSON encoded string to an Expression object. */
+  public static ExprValue setJson(ExprValue json, ExprValue path, ExprValue valueToInsert) {


What about the case where the JSON path points to multiple places? Does this method support inserting multiples values (it doesn't seem like it)? Would it just insert the first one, would it fail? Is this a case we should consider, and have an appropriate error message for it?

currantw · 2025-01-31T22:33:25Z

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+        case Iterable<?> ignored -> {
+          // New element in the array.
+          String updatedJson = docContext.add(pathUnquoted, valueUnquoted).jsonString();
+          return new ExprStringValue(updatedJson);
+        }


Am I understanding this correctly? If I call jsonSet with a JSON path that points to an array, this will insert the given value into the array, rather than set it equal to the given value?

What about the case where the JSON path points to a nested JSON object? Is a JSON object Iterable, and so would it fall into this case?

currantw · 2025-01-31T22:39:37Z

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+  /**
+   * Helper method to handle recursive scenario.
+   *
+   * @param docContext incoming Json in Java object form.
+   * @param path path in String to perform insertion.
+   * @param value value to be inserted with given path.
+   */
+  private static void recursiveCreate(DocumentContext docContext, String path, Object value) {
+    final int pos = path.lastIndexOf('.');
+    final String parent = path.substring(0, pos);
+    final String current = path.substring(pos + 1);
+    // Attempt to read the current path as it is, trigger the recursive in case of deep insert.
+    if (docContext.read(parent) == null) {
+      recursiveCreate(docContext, parent, new LinkedHashMap<>());
+    }
+    docContext.put(parent, current, value);
+  }


As mentioned in another comment, have you verified that this (recursive insertion) is not handled by WriteContext.set?

currantw · 2025-01-31T22:47:46Z

core/src/test/java/org/opensearch/sql/expression/json/JsonFunctionsTest.java

+  void json_set_InsertString() {
+    FunctionExpression functionExpression =
+        DSL.jsonSet(DSL.literal("[]"), DSL.literal("$"), DSL.literal("test_value"));
+    assertEquals("[\"test_value\"]", functionExpression.valueOf().stringValue());
+  }


Might be worth extracting a helper function? Could help make this a big easier to read?

void json_set_InsertString() { assertEquals("[\"test_value\"]", execute_set_json("[]", "$", "test_value")); } static String execute_set_json(String jsonString, String jsonPathString, Object value) { ExprValue jsonExprValue = ExprValueUtils.stringValue(jsonString); ExprValue jsonPathExprValue = ExprValueUtils.stringValue(jsonPathString); ExprValue setExprValue = ExprValueUtils.fromObjectValue(value); FunctionExpression functionExpression = DSL.jsonSet(jsonExprValue, jsonPathExprValue, setExprValue); return functionExpression.valueOf().stringValue(); }

currantw · 2025-01-31T22:50:55Z

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: JSON_STRING, STRING, STRING/BOOLEAN/DOUBLE/INTEGER/NULL/STRUCT/ARRAY


Looks like you need to update this to include a few more supported types? Also, shouldn't this just be STRING instead of JSON_STRING?

currantw · 2025-01-31T22:51:20Z

docs/user/ppl/functions/json.rst

+
+Argument type: JSON_STRING, STRING, STRING/BOOLEAN/DOUBLE/INTEGER/NULL/STRUCT/ARRAY
+
+Return type: JSON_STRING


Similarly, should this just be STRING?

currantw · 2025-01-31T22:54:54Z

integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionsIT.java

+            String.format(
+                "source=%s | eval updated=json_set(json_string, \\\"$.c.innerProperty\\\","
+                    + " \\\"test_value\\\") | fields test_name, updated",
+                TEST_INDEX_JSON_TEST));


I curious: why do we need so many escape characters here? Why doesn't one suffice?

14yapkc1 and others added 30 commits January 3, 2025 10:54

added implementation

70152eb

Signed-off-by: Kenrick Yap <[email protected]>

added doctest, integ-tests, and unit tests

76c3995

Signed-off-by: Kenrick Yap <[email protected]>

addressed PR comments

ce2c551

Signed-off-by: Kenrick Yap <[email protected]>

fixed unit tests

ad1bde3

Signed-off-by: Kenrick Yap <[email protected]>

addressed pr comments

ccf47a2

Signed-off-by: Kenrick Yap <[email protected]>

addressed PR comments

acc76a0

Signed-off-by: Kenrick Yap <[email protected]>

removed unused dependencies

519c6f2

Signed-off-by: Kenrick Yap <[email protected]>

linting

2e319fe

Signed-off-by: Kenrick Yap <[email protected]>

addressed pr comment and rolling back disabled test case

ee0820d

Signed-off-by: Kenrick Yap <[email protected]>

Merge branch 'main' into feature/json-valid

d44fc5a

Signed-off-by: Kenrick Yap <[email protected]>

removed disabled import

3407d4a

Signed-off-by: Kenrick Yap <[email protected]>

Update docs/user/ppl/functions/json.rst

7ef6cc9

Co-authored-by: Andrew Carbonetto <[email protected]> Signed-off-by: kenrickyap <[email protected]>

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]>

nit

2187a5a

Signed-off-by: Kenrick Yap <[email protected]>

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]>

fixed integ test

3512b33

Signed-off-by: Kenrick Yap <[email protected]>

change text type to keyword

9fea606

Signed-off-by: Kenrick Yap <[email protected]>

addressed PR comments

fbc54bc

Signed-off-by: Kenrick Yap <[email protected]>

fix doc-test

31ad2a4

Signed-off-by: Kenrick Yap <[email protected]>

added null test

2b2a8f3

Signed-off-by: Kenrick Yap <[email protected]>

Merge branch 'main' into feature/json-valid

dc96563

SQL: adding error case unit tests for json_valid

1913bfe

Signed-off-by: Andrew Carbonetto <[email protected]>

json_valid: null and missing should return false

67d979d

Signed-off-by: Andrew Carbonetto <[email protected]>

PPL: Add json and cast to json functions

aa6b723

Signed-off-by: Andrew Carbonetto <[email protected]>

PPL: Update json cast for review

4c99235

Signed-off-by: Andrew Carbonetto <[email protected]>

Fix testes

9ccde7f

Signed-off-by: Andrew Carbonetto <[email protected]>

spotless

4306bf3

Signed-off-by: Andrew Carbonetto <[email protected]>

Fix tests

613137b

Signed-off-by: Andrew Carbonetto <[email protected]>

SPOTLESS

ab28872

Signed-off-by: Andrew Carbonetto <[email protected]>

Clean up for merge

3ec16e0

Signed-off-by: Andrew Carbonetto <[email protected]>

andy-k-improving requested review from kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, mengweieric, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224, LantaoJin and noCharger as code owners January 28, 2025 18:54

andy-k-improving changed the title ~~Ft ak json set~~ Feature: PPL - json_set function Jan 28, 2025

normanj-bitquill reviewed Jan 30, 2025

View reviewed changes

async-query-core/src/main/antlr/OpenSearchPPLLexer.g4 Outdated Show resolved Hide resolved

normanj-bitquill reviewed Jan 30, 2025

View reviewed changes

async-query-core/src/main/antlr/OpenSearchPPLParser.g4 Outdated Show resolved Hide resolved

normanj-bitquill reviewed Jan 30, 2025

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/json/JsonFunctions.java Outdated Show resolved Hide resolved

normanj-bitquill reviewed Jan 30, 2025

View reviewed changes

core/src/main/java/org/opensearch/sql/utils/JsonUtils.java Outdated Show resolved Hide resolved

normanj-bitquill reviewed Jan 30, 2025

View reviewed changes

docs/user/ppl/functions/json.rst Outdated Show resolved Hide resolved

andy-k-improving added 2 commits January 31, 2025 10:47

Code review

3db65c3

Signed-off-by: Andy Kwok <[email protected]>

Fix doc test

2509fc8

Signed-off-by: Andy Kwok <[email protected]>

currantw reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: PPL - json_set function #3271

Feature: PPL - json_set function #3271

andy-k-improving commented Jan 28, 2025 •

edited

Loading

YANG-DB commented Jan 28, 2025

andy-k-improving commented Jan 28, 2025 •

edited

Loading

andy-k-improving commented Jan 31, 2025

currantw left a comment

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

currantw Jan 31, 2025

		String jsonUnquoted = StringUtils.unquoteText(json.stringValue());
		String pathUnquoted = StringUtils.unquoteText(path.stringValue());


		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: JSON_STRING, STRING, STRING/BOOLEAN/DOUBLE/INTEGER/NULL/STRUCT/ARRAY


		Argument type: JSON_STRING, STRING, STRING/BOOLEAN/DOUBLE/INTEGER/NULL/STRUCT/ARRAY

		Return type: JSON_STRING

Feature: PPL - json_set function #3271

Are you sure you want to change the base?

Feature: PPL - json_set function #3271

Conversation

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

Description

Related Issues

Check List

YANG-DB commented Jan 28, 2025

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

andy-k-improving commented Jan 31, 2025

currantw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-k-improving commented Jan 28, 2025 •

edited

Loading

andy-k-improving commented Jan 28, 2025 •

edited

Loading