Add Support for Nested Function Use In WHERE Clause

[forestmvey]

Description

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

Add support for use of the nested function in the WHERE clause as a predicate expression. Supports nested function on left of operator, and literal on right. When using nested function in WHERE clause, the inner hits of the query are not added to the nested query DSL. To do this you must use the nested function in conjunction with a SELECT clause nested function call. See documentation for this nested implementation HERE (WIP).

Example Queries

SELECT message.info FROM nested_objects WHERE nested(message.info, message) = 'a';
SELECT nested(message.info) FROM nested_objects WHERE nested(message.info, message) = 'a';
SELECT message.info FROM nested_objects WHERE nested(message.info) = 'a' OR nested(comment.data) = 'b' AND nested(message.dayOfWeek) = 4;

Issues Resolved

Issue: 1111

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]

Codecov Report

Merging #267 (6881b3a) into integ-nested-where-clause-predicate-expression (8e5d766) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@                                 Coverage Diff                                  @@
##             integ-nested-where-clause-predicate-expression     #267      +/-   ##
====================================================================================
+ Coverage                                             97.16%   97.18%   +0.01%     
- Complexity                                             4120     4150      +30     
====================================================================================
  Files                                                   371      372       +1     
  Lines                                                 10373    10429      +56     
  Branches                                                704      716      +12     
====================================================================================
+ Hits                                                  10079    10135      +56     
  Misses                                                  287      287              
  Partials                                                  7        7              

Flag	Coverage Δ
sql-engine	97.18% <100.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
...ain/java/org/opensearch/sql/analysis/Analyzer.java	100.00% <ø> (ø)
...l/opensearch/request/OpenSearchRequestBuilder.java	100.00% <100.00%> (ø)
...arch/storage/script/filter/FilterQueryBuilder.java	100.00% <100.00%> (ø)
...search/storage/script/filter/lucene/LikeQuery.java	100.00% <100.00%> (ø)
...arch/storage/script/filter/lucene/LuceneQuery.java	100.00% <100.00%> (ø)
...arch/storage/script/filter/lucene/NestedQuery.java	100.00% <100.00%> (ø)
...java/org/opensearch/sql/sql/parser/AstBuilder.java	100.00% <100.00%> (ø)

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

[matthewryanwells]

What does this lambda function do? How is it different from a without the lambda function around it such as:

NestedQueryBuilder nestedQuery = nestedQuery(path, matchAllQuery(), ScoreMode.None);
      ((BoolQueryBuilder) query().filter().get(0)).must(nestedQuery);
      return nestedQuery;

[forestmvey]

Good question. The Supplier is an interface to fulfill the orElseGet() call in findNestedQueryWithSamePath(). We don't want to use orElse() and not use the Supplier cause that would have the createEmptyNestedQuery() code always execute. Instead we use orElseGet() which takes the Supplier interface and avoids unnecessary code execution with a non-empty optional from findAny().

You can read more about this here:
https://www.baeldung.com/java-optional-or-else-vs-or-else-get

[MaxKsyunz]

Have you considered implementing nested similarly to relevance search functions?

In particular it's unexpected that generic classes like LuceneQuery and OpenSearchRequestBuilder needed changes.

I'd like to understand this before approving this PR.

[GumpacG]

If nested is supported in functions for WHERE clause, could you also add IT for these cases? For example, SELECT * FROM nested WHERE nested(message.info) LIKE 'a'.

[dai-chen]

I'm also trying to figure out best way. As I understand, what we want to do is:

if isNestedFunc(left) {
  // convert left from nested(A) to A and keep others the same?
  return new NestedQuery(
    visitFunction(
      new Function(func.name(), left.argument(0), right)));
}

Is it a little better to move all logic to a NestedQuery class?

[forestmvey]

This is close to a solution. We could swap around the arguments in the ExpressionAnalyzer:visitFunction to turn this function:
Function(=) : { nested(...), literal(...)}
Into this:
Function(nested) : { function(=) : { nested(...), literal(...)}

If we do something like this we can move most of the logic to a class extension of LuceneQuery and override a build() function. It's a tradeoff as we will need logic to handle only this case in ExpressionAnalyzer and the overriding class handling the nested specific logic will need to use the luceneQueries map to build the query function. The code isn't very extensible in pushDownFilter for changing the left parameter of a predicate expression to something other than a ReferenceExpression.

Both implementations will have their drawbacks but without a refactor I can't see a clean implementation for this syntax. At least this isn't a large change that would require much work to refactor.

[forestmvey]

I think I prefer the current solution in this PR more as it may be easier to refactor later if needed. I find swapping the expression tree more clunky with the only benefit being able to move logic to NestedQuery class. Do you have an opinion @dai-chen or any other comments?

[dai-chen]

Sorry for confusion. I'm not suggesting to do this in ExpressionAnalyzer. The pseudocode above is just for this class at this line. I'm just thinking can we move handleNestedPredicateExpression, getNestedPathString, buildNested to a NestedQuery class extends LuceneQuery?

[forestmvey]

Thanks for the clarification. I found a way to move all logic to a NestedQuery class extension of LuceneQuery. Thanks for your input.

[dai-chen]

This is more like a util method which seems not supposed to be a new API?

[GumpacG]

nit: could do something like

List<String> tmpPaths = field.getPaths();
tmpPaths.remove(tmpPaths.size()-1);
return String.join(".", tmpPaths);

[acarbonetto]

nit: Need to wrap it inside an if statement to make sure there's at least one item in the list (for Guian's suggestion).

[acarbonetto]

alternatively, use a .stream().filter(check for instance).findFirst().isPresent(throw)