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

PPL: Add json_extract function #3262

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

Conversation

kenrickyap
Copy link
Contributor

@kenrickyap kenrickyap commented Jan 23, 2025

Description

Implements json_extract function that would be useful to extract specific objects and scalars from an existing json_object.

Related Issues

Resolves #3211
Follow up issue for #3027

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
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
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]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
Signed-off-by: Andrew Carbonetto <[email protected]>
@kenrickyap
Copy link
Contributor Author

would be ideal to merge:

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 0f0b8d4a26fa47d2f9ae190d9050d9d87b2ab37c.

Signed-off-by: 14yapkc1 <[email protected]>
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 f030057

docs/user/ppl/functions/json.rst 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
docs/user/ppl/functions/json.rst Show resolved Hide resolved
Signed-off-by: 14yapkc1 <[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
#	core/src/test/java/org/opensearch/sql/expression/operator/convert/TypeCastOperatorTest.java
#	docs/user/ppl/functions/json.rst
#	integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionsIT.java
#	ppl/src/main/antlr/OpenSearchPPLLexer.g4
#	ppl/src/main/antlr/OpenSearchPPLParser.g4
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 2b08007.

kenrickyap and others added 2 commits January 29, 2025 11:11
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
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 e57fa21.

docs/user/ppl/functions/json.rst 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
…tionsTest.java

Co-authored-by: Taylor Curran <[email protected]>
Signed-off-by: kenrickyap <[email protected]>
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 dfe5d3a. Looks good to me.

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 dfe5d3a. Looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

can u plz add license header ?
thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

YANG-DB
YANG-DB previously approved these changes Jan 30, 2025
Signed-off-by: Kenrick Yap <[email protected]>
Signed-off-by: Kenrick Yap <[email protected]>
@kenrickyap kenrickyap force-pushed the feature/json-extract branch from dfe5d3a to 75e9cc3 Compare January 31, 2025 14:57
Signed-off-by: Kenrick Yap <[email protected]>
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

a few comments to address

private DefaultFunctionResolver jsonExtract() {
return define(
BuiltinFunctionName.JSON_EXTRACT.getName(),
impl(JsonUtils::extractJson, UNDEFINED, STRING, STRING));
Copy link
Collaborator

Choose a reason for hiding this comment

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

try {
Object results = JsonPath.parse(jsonString).read(jsonPath);
return ExprValueUtils.fromObjectValue(results);
} catch (PathNotFoundException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} catch (PathNotFoundException e) {
} catch (PathNotFoundException ignored) {

return ExprValueUtils.fromObjectValue(results);
} catch (PathNotFoundException e) {
return LITERAL_NULL;
} catch (InvalidPathException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don't reuse variable names:

Suggested change
} catch (InvalidPathException e) {
} catch (InvalidPathException invalidPathException) {

} catch (InvalidPathException e) {
final String errorFormat = "JSON path '%s' is not valid. Error details: %s";
throw new SemanticCheckException(String.format(errorFormat, path, e.getMessage()), e);
} catch (InvalidJsonException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don't reuse variable names:

Suggested change
} catch (InvalidJsonException e) {
} catch (InvalidJsonException invalidJsonException) {

@Test
void json_extract_search() {
ExprValue expected = new ExprIntegerValue(1);
execute_extract_json(expected, "{\"a\":1}", "$.a");
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add a test for each scalar type: boolean, double, string, integer (already have) and null (not exactly the same case as the test case below).


@Test
void json_extract_search_arrays_out_of_bound() {
execute_extract_json(LITERAL_NULL, "{\"a\":[1,2,3]}", "$.a[4]");
Copy link
Collaborator

Choose a reason for hiding this comment

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

move to `json_extract_returns_null

// null json
assertEquals(
LITERAL_NULL,
DSL.jsonExtract(DSL.literal(LITERAL_NULL), DSL.literal(new ExprStringValue("$.a")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DSL.jsonExtract(DSL.literal(LITERAL_NULL), DSL.literal(new ExprStringValue("$.a")))
DSL.jsonExtract(DSL.literal(LITERAL_NULL), DSL.literal("$.a"))

// missing json
assertEquals(
LITERAL_MISSING,
DSL.jsonExtract(DSL.literal(LITERAL_MISSING), DSL.literal(new ExprStringValue("$.a")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
DSL.jsonExtract(DSL.literal(LITERAL_MISSING), DSL.literal(new ExprStringValue("$.a")))
DSL.jsonExtract(DSL.literal(LITERAL_MISSING), DSL.literal("$.a"))

Copy link
Collaborator

Choose a reason for hiding this comment

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

same for rest of file...

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.

[FEATURE] Add json_extract function to PPL
6 participants