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 Nested Function Support in SELECT Clause #243

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

forestmvey
Copy link

@forestmvey forestmvey commented Mar 9, 2023

Description

Syntax: nested( [field} | [field,path] )

Nested function allows the query of nested types of an index. The path parameter is determined dynamically if not provided by the user, and the output query should be identical assuming the user input the correct parameter values. The condition parameter is not supported when the nested function is used in the SELECT clause. Nested types structure is flattened making the full path of an object the key, and the object it refers to the value.

   Sample input=
   keys = ['comments.likes']
   row = comments: {
   likes: 2
   }
   output =
   flattenedRow = {comment.likes: 2}

Example Queries

Simple nested query, array of message.info is flattened into two rows.

SELECT nested(message.info) FROM nested_objects;

After flattening notice comment.data has repeating ab

SELECT nested(message.info), nested(comment.data) FROM nested_objects;

someField is of type object

SELECT nested(message.info), someField FROM nested_objects;

To Do

  • nested function nested in other function call (Not currently supported in legacy)

Not Supported

  • Using condition parameter in the SELECT clause.

Changes from Legacy Functionality

  • Nested function does not support condition parameter in SELECT clause.
  • Updated partiql.rst to handle object arrays same as V2 engine.

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

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.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #243 (9b803d4) into integ-nested-select-clause (23cc0f6) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 9b803d4 differs from pull request most recent head 48553f3. Consider uploading reports for the commit 48553f3 to get more accurate results

@@                       Coverage Diff                        @@
##             integ-nested-select-clause     #243      +/-   ##
================================================================
+ Coverage                         98.46%   98.50%   +0.03%     
- Complexity                         3869     3964      +95     
================================================================
  Files                               345      351       +6     
  Lines                              9603     9845     +242     
  Branches                            616      653      +37     
================================================================
+ Hits                               9456     9698     +242     
  Misses                              142      142              
  Partials                              5        5              
Flag Coverage Δ
sql-engine 98.50% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java 100.00% <ø> (ø)
...ch/sql/planner/optimizer/LogicalPlanOptimizer.java 100.00% <ø> (ø)
...ain/java/org/opensearch/sql/analysis/Analyzer.java 100.00% <100.00%> (ø)
...va/org/opensearch/sql/analysis/NestedAnalyzer.java 100.00% <100.00%> (ø)
...c/main/java/org/opensearch/sql/expression/DSL.java 100.00% <100.00%> (ø)
...opensearch/sql/expression/ReferenceExpression.java 100.00% <100.00%> (ø)
...h/sql/expression/function/BuiltinFunctionName.java 100.00% <100.00%> (ø)
...ql/expression/function/NestedFunctionResolver.java 100.00% <100.00%> (ø)
...h/sql/expression/function/OpenSearchFunctions.java 100.00% <100.00%> (ø)
...ensearch/sql/expression/nested/NestedFunction.java 100.00% <100.00%> (ø)
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Cool!
Doctests? Demo? Nested party?

@forestmvey forestmvey marked this pull request as ready for review March 20, 2023 18:38
@forestmvey forestmvey requested a review from a team March 20, 2023 18:38
@GumpacG
Copy link

GumpacG commented Mar 20, 2023

Nested with date type subfields are supported however it returns the wrong type. Maybe date types need to be included in NestedFunction.java.
Legacy:
Screenshot 2023-03-20 at 3 32 13 PM
V2:
Screenshot 2023-03-20 at 3 32 28 PM

@forestmvey forestmvey force-pushed the dev-nested-cleanup branch 2 times, most recently from e5221ba to 03cd356 Compare March 22, 2023 16:15
@GabeFernandez310
Copy link

Does documentation/doctests need to be added?

result = flatten(field, inputValue, result, true);
}

if (result.isEmpty()) {

Choose a reason for hiding this comment

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

when does this happen?

Copy link
Author

@forestmvey forestmvey Mar 27, 2023

Choose a reason for hiding this comment

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

Kept in place in-case of null values in data.

ExprValue row,
List<Map<String,
ExprValue>> prevList,
boolean supportArrays

Choose a reason for hiding this comment

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

can we make supportArrays == false a separate function? Feels like these have different behaviours anyways

Copy link
Author

Choose a reason for hiding this comment

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

We can but we will need another function that is almost the same as getNested.

@forestmvey
Copy link
Author

Does documentation/doctests need to be added?

Added doctest in 10ee84d14f28b1c7e5bfa92a76e92cf9c2d74579

@forestmvey forestmvey force-pushed the dev-nested-cleanup branch 3 times, most recently from c7229e9 to 26aa7c6 Compare March 28, 2023 22:17
NestedAnalyzer nestedAnalyzer = new NestedAnalyzer(
namedExpressions, expressionAnalyzer, child
);
child = nestedAnalyzer.analyze(expr, context);

Choose a reason for hiding this comment

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

You overwrite child set by highlightAnalyzer, which overwrites child set by windowAnalyzer, which .... Is it safe? Is nested compatible with highlight?
Do you have [IT] tests for that?

Copy link
Author

Choose a reason for hiding this comment

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

Don't believe we have any requirement for nested to work with added V2 functionality like highlight. Goal is legacy deprecation and we want to abandon the NLPChina implementation for a better standard like PartiQL. I can pushDown highlight and nested, I'm not sure what a valid query would be though?

Choose a reason for hiding this comment

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

select nested(...), highlight(...) from ... where match(...)
Can you try this?

Copy link
Author

@forestmvey forestmvey Mar 30, 2023

Choose a reason for hiding this comment

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

Not working with Relevance Queries. Issue here for tracking.
1488

docs/user/dql/functions.rst Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
doctest/test_mapping/nested_objects.json Show resolved Hide resolved
sql/src/main/antlr/OpenSearchSQLParser.g4 Show resolved Hide resolved
@forestmvey forestmvey force-pushed the dev-nested-cleanup branch 2 times, most recently from 19f5c95 to dc718e2 Compare March 29, 2023 20:24
@forestmvey forestmvey merged commit 4982979 into integ-nested-select-clause Mar 30, 2023
@forestmvey forestmvey deleted the dev-nested-cleanup branch March 30, 2023 21:18
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.

6 participants