#639: allow metadata fields and score opensearch function #228

acarbonetto · 2023-02-15T19:57:14Z

Description

OpenSearch reserved fields (_id, _index, _sort, _score, _max_score) are not allowed to be used in SQL clauses (SELECT, WHERE, ORDER BY) because the field format starting with underscore _ is not allowed.

This ticket adds specific identifiers to the language, and opens up support for OpenSearch reserved identifiers.
As an aside, identifiers with double underscore at the start (such as __myCoolField) is acceptable as an identifier.

This ticket allows for the score(), score_query() and scorequery() function to be used to wrap around relevance-search queries to force the _score and _max_score metadata fields to be returned with values. For some queries, _score returns null unless the score() function is included.

The score function also allows for an optional boost argument to be included that boosts the score of the child relevance function.

Example - metadata fields returned:

SELECT calcs.key, str0, _id, _sort, _score, _maxscore FROM calcs WHERE _id="5"
Result:
{ "key04", "OFFICE SUPPLIES", "5", -2, null, null }

Example - Metadata fields not requested are not displayed:

SELECT *, _id FROM bigint WHERE _id="2"
Result (assuming bigint only has one field):
[9223372026854775807, "2"]

Example - relevance search without and with score function

SELECT _id, _index, _score, _maxscore, str0, str1 FROM calcs WHERE QUERY_STRING([\"*\"], `BINDING SUPPLIES`);
result: 
[
    "7",
    "calcs",
    null,
    null,
    "OFFICE SUPPLIES",
    "BINDING SUPPLIES"
]

SELECT _id, _index, _score, _maxscore, str0, str1 FROM calcs WHERE SCORE(QUERY_STRING([\"*\"], `BINDING SUPPLIES`));
[
    "7",
    "calcs",
    2.4849067,
    2,
    "OFFICE SUPPLIES",
    "BINDING SUPPLIES"
]

Example - boost score on the _sql/_explain call:

SELECT _id, _score, _maxscore FROM calcs WHERE SCORE(QUERY_STRING([\"*\"], `BINDING SUPPLIES`, boost=2.5));
result: 
[
    "7",
    6.2122664,
    6
]

SELECT _id, _score, _maxscore FROM calcs WHERE SCOREQUERY(QUERY_STRING([\"*\"], `BINDING SUPPLIES`, boost=2.5), 2.0);
result: 
[
    "7",
    12.424533,
    12
]

SELECT _id, _index, _score, _maxscore, str0, str1 FROM calcs WHERE SCORE_QUERY(QUERY_STRING([\"*\"], `BINDING SUPPLIES`), 2.5);
result:
[
    "7",
    6.2122664,
    6
]

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 · 2023-02-15T20:13:09Z

Codecov Report

❗ No coverage uploaded for pull request base (integ-metadata-fields@d44cd39). Click here to learn what that means.
The diff coverage is n/a.

@@                   Coverage Diff                    @@
##             integ-metadata-fields     #228   +/-   ##
========================================================
  Coverage                         ?   98.47%           
  Complexity                       ?     3875           
========================================================
  Files                            ?      345           
  Lines                            ?     9648           
  Branches                         ?      626           
========================================================
  Hits                             ?     9501           
  Misses                           ?      142           
  Partials                         ?        5

Flag	Coverage Δ
sql-engine	`98.47% <0.00%> (?)`

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

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

...rc/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java

dai-chen · 2023-02-16T21:55:05Z

not sure if opensearch-project#788 can be resolved as well?

acarbonetto · 2023-02-16T22:37:08Z

not sure if opensearch-project#788 can be resolved as well?

This problem will occur for any field that shares a name with a function. The workaround for now will be to use backticks.

dai-chen · 2023-02-17T18:55:30Z

not sure if opensearch-project#788 can be resolved as well?

This problem will occur for any field that shares a name with a function. The workaround for now will be to use backticks.

I think the issue you're referring to is already fixed here: opensearch-project#1191. I tried to fix score in this PR but got some problem.

dai-chen · 2023-02-17T18:59:17Z

core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java

+   * @param context analysis context for the query
+   * @return resolved relevance function
+   */
+  public Expression visitScoreFunction(ScoreFunction node, AnalysisContext context) {


I think we need to add this to opensearch module as storage-specific function. Personally I think we should prioritize opensearch-project#811. It will become more and more difficult as we keep adding more OpenSearch logic to core engine.

Agreed. I've got it on our radar, and we can start to scope it out. As is, there's quite a bit of work to do to pull out the opensearch specific classes, but I think it's do-able.

We've had storage-specific function support in opensearch-project#1354. Can we start this now instead of adding more and more special logic to core? Agreed there is quite lots of work to move all to opensearch but maybe easy to do this for single PR?

Let me take a look. There's already a lot in this PR.
Would you mind if we move the score function out as a proof of concept for a set of OpenSearch storage engine functions?

dai-chen · 2023-02-17T19:03:48Z

core/src/main/java/org/opensearch/sql/ast/expression/QualifiedName.java

+  public static final String METADATA_FIELD_SCORE = "_score";
+  public static final String METADATA_FIELD_MAXSCORE = "_maxscore";
+  public static final String METADATA_FIELD_SORT = "_sort";
+  public static final java.util.Map<String, ExprCoreType> METADATAFIELD_TYPE_MAP = new HashMap<>() {


We may not want to hardcode this in core. Not sure if adding these fields in OpenSearchIndex.getFieldMapping() can work for you, however we need to consider meta column in other storage engine. For example, S3 may have $path, $partition in each row.

I can't use OpenSearchIndex as it would create a dependency on opensearch in core. Not ideal. We kind of need to wait until we have a user-defined functions that could override core visitors, like QualifiedName.

Sorry, I didn't mean you need to depend on opensearch module. OpenSearchIndex is subclass of our Table interface. As I understand, Analyzer will fetch field list from Table during query analysis. I saw you've added isMetaField flag, so I don't see why we want to hardcode metadata field list in core module.

Oh. Interesting. I'll see if I can hook that up.

Please see changes in:

core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java (for interface changes in the environment)

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java (specific changes for OpenSearch indexes), and

core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java the visitQualifiedName function

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

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

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

MaxKsyunz · 2023-03-13T22:15:19Z

opensearch/src/test/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClientTest.java

    Iterator<ExprValue> hits = response1.iterator();
    assertTrue(hits.hasNext());
-    assertEquals(exprTupleValue, hits.next());
+    assertEquals(exprTupleValue.tupleValue().get("id"), hits.next().tupleValue().get("id"));


Why was this change necessary?

it was matching the whole object - and failing because the Boolean object wasn't .equal. This could be removed (I think) if I change the Boolean to a boolean

Sorry - I got that wrong. We are now returning the metafields in the response from OpenSearch.. so they're appearing in the tuple. This way the spirit of the test remains the same.

sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java

core/src/main/java/org/opensearch/sql/ast/expression/QualifiedName.java

docs/user/dql/functions.rst

MaxKsyunz · 2023-03-14T01:35:10Z

integ-test/src/test/java/org/opensearch/sql/sql/IdentifierIT.java

+  }
+
+  @Test
+  public void testMetafieldIdentifierTest() throws IOException {


What happens when getting a meta field from a prometheus data source?

The parser will fail. There's no workaround for prometheus data sources for _id, _score, _index, _maxscore and _sort. But it will work for other fields that begin with an underscore (e.g. _routing or __fieldname) that is not strictly defined as an opensearch meta-field.
In other words: this is an improvement on the existing functionality... which would just fail for ALL fields starting with an underscore.

MaxKsyunz · 2023-03-14T08:34:49Z

docs/user/dql/functions.rst

+``score_query(search_expression, boost)``
+``scorequery(search_expression, boost)``
+
+The score function returns the _score of any documents matching the enclosed relevance-search expression. The SCORE function expects two


As a users it is unexpected that SELECT _score, phrase FROM phrases returns _score value yet SELECT _score, phrase FROM phrases WHERE match('phrase', 'my') returns _score of null.

I'd expect _score to be returned if requested since it can be calculated for any OpenSearch query.

This is an OpenSearch value. I would think users need to understand the OpenSearch side of things. Once could ask why OpenSearch doesn't also just calculate the score for everything...

The plugin could also set track_scores = true if the user asks for _score field and is using relevance search.

At the very least, it should be mentioned here that _score will be null unless relevance query is wrapped in score(...).

sql/src/main/antlr/OpenSearchSQLParser.g4

...rc/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java

core/src/main/java/org/opensearch/sql/expression/function/OpenSearchFunctions.java

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

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndex.java

sql/src/main/antlr/OpenSearchSQLParser.g4

core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java

core/src/main/java/org/opensearch/sql/storage/Table.java

docs/user/dql/functions.rst

core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java

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

MaxKsyunz · 2023-03-21T07:03:02Z

core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java

  public TypeEnvironment(TypeEnvironment parent, SymbolTable symbolTable) {
    this.parent = parent;
    this.symbolTable = symbolTable;
+    this.reservedSymbolTable = new SymbolTable();


Why is a separate symbol table needed? Why can't we add metafields to symbolTable?

To separate out 'reserved fields' (that are in all tables) from fields that are specific to the index. The code needs to know the difference between these two lists later on.

…ore OpenSearch function (#228) (opensearch-project#1456) Allow metadata fields and score OpenSearch function. Signed-off-by: Andrew Carbonetto <[email protected]>

…ore OpenSearch function (#228) (opensearch-project#1456) Allow metadata fields and score OpenSearch function. Signed-off-by: Andrew Carbonetto <[email protected]> (cherry picked from commit e805151)

…e opensearch function (#228) (opensearch-project#1508) * opensearch-project#639: Support OpenSearch metadata fields and the score OpenSearch function (#228) (opensearch-project#1456) Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]>

…e opensearch function (#228) (opensearch-project#1509) * opensearch-project#639: Support OpenSearch metadata fields and the score OpenSearch function (#228) (opensearch-project#1456) Signed-off-by: Andrew Carbonetto <[email protected]> Co-authored-by: Andrew Carbonetto <[email protected]>

…elds and the score OpenSearch function (#228) (opensearch-project#1456) Allow metadata fields and score OpenSearch function. Signed-off-by: Andrew Carbonetto <[email protected]>

Yury-Fridlyand reviewed Feb 16, 2023

View reviewed changes

...rc/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanQueryBuilder.java Outdated Show resolved Hide resolved

acarbonetto marked this pull request as ready for review February 17, 2023 17:09

dai-chen reviewed Feb 17, 2023

View reviewed changes

acarbonetto force-pushed the dev-metadata-fields branch 2 times, most recently from b4912da to c8f0520 Compare March 7, 2023 22:18

acarbonetto added 21 commits March 7, 2023 14:20

Rebase from main

d8b412d

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

Update to define and include metadata when visiting the expr node

5eaa344

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

Add specific metadata identifiers

ea25455

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

Add IT tests and add parser changes

9f1dcca

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

Rebase from main

5a70363

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

Update score function expression analyzer to return boosted relevance…

8c0aaf6

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

Update builder to track scores

4d8229d

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

Remove ScoreExpression.java and cleanup checkstyle

4630c98

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

cleanup checkstyle

9a10273

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

Cleanup and add alternative score function syntax

0a92969

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

Cleanup and add alternative score function syntax

3b5e900

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

Fix some bugs and add Expression tests

348b8b9

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

Add expresssion and analyzer tests

f67d4f2

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

Add score doctests

be7191a

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

Add score function doctests

1a0b3ef

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

Add metafield tests

05d9dd3

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

Move legacy test and mark old as ignore

996a166

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

fix checkstyle violations

2c469c7

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

fix checkstyle violations

4c7a5d7

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

Update tests and identifier to accept metafields

dec36fa

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

Checkstyle fixes

7aab6ee

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

acarbonetto added 2 commits March 13, 2023 17:44

Update basics.rst to show OS result

7a05276

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

Update basics.rst to show OS result

e75d6b5

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

MaxKsyunz reviewed Mar 14, 2023

View reviewed changes

GabeFernandez310 approved these changes Mar 14, 2023

View reviewed changes

Update basics.rst description

fecf615

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

Yury-Fridlyand approved these changes Mar 14, 2023

View reviewed changes

acarbonetto added 5 commits March 14, 2023 17:04

Change Score function to accept a double/integer not an unresolved

47636c3

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

Update functions.rst

6d9853c

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

Checkstyle update

f93f7c4

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

Move reserved world symbol table to OpenSearchTable

1340f11

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

Update functions.rst for review comments

bdc3020

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

Yury-Fridlyand approved these changes Mar 16, 2023

View reviewed changes