Add Highlight In SQL #96

forestmvey · 2022-07-26T17:25:42Z

Description

Support highlighting function in SQL query engine.

Issues Resolved

Github Issue: 636

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.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

In particular, highlightField is an expression now. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

HighlightFunction is converted to LogicalHighlight logical plan. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Yury-Fridlyand · 2022-07-27T00:07:45Z

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

+  /**
+   * Return String or Map value matching highlight field.
+   * @param valueEnv : Dataset to parse value from.
+   * @return : String or Map value of highlight fields.
+   */
+  @Override
+  public ExprValue valueOf(Environment<Expression, ExprValue> valueEnv) {
+    String refName = "_highlight";
+    if (!getHighlightField().toString().contains("*")) {
+      refName += "." + StringUtils.unquoteText(getHighlightField().toString());
+    }
+    ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING));
+    ImmutableMap.Builder<String, ExprValue> builder = new ImmutableMap.Builder<>();
+
+    if (retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) {
+      return retVal;
+    }
+
+    var hlBuilder = ImmutableMap.<String, ExprValue>builder();
+    hlBuilder.putAll(retVal.tupleValue());
+
+    for (var entry : retVal.tupleValue().entrySet()) {
+      String entryKey = "highlight(" + getHighlightField() + ")." + entry.getKey();
+      builder.put(entryKey, ExprValueUtils.stringValue(entry.getValue().toString()));
+    }
+
+    return ExprTupleValue.fromExprValueMap(builder.build());
+  }


Is it worth returning always a map?

Line 55 returns only one field in the instance there aren't multiple highlight fields returned.

Yury-Fridlyand · 2022-07-27T00:08:42Z

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

+    ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING));
+    ImmutableMap.Builder<String, ExprValue> builder = new ImmutableMap.Builder<>();
+
+    if (retVal.isMissing() || retVal.type() != ExprCoreType.STRUCT) {


When this condition is true?

If there was no highlight value parsed from the return, or only a single highlight field is found.

Yury-Fridlyand · 2022-07-27T00:12:05Z

core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java

+public class HighlightOperator extends PhysicalPlan {
+  @NonNull
+  private final PhysicalPlan input;


Why do you store a parent class instance there?
I think you can call super.next() and super.hasNext() instead of input.next() and input.hasNext() and so on.

In line with Peng's comment in the POC, this class ended up not doing anything and can be removed.

Seems to be standard considering other operators also store the parent class instance like this.

Yury-Fridlyand · 2022-07-27T00:13:09Z

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

@@ -274,7 +292,7 @@ public void project_values() {
        AstDSL.project(
            AstDSL.values(ImmutableList.of(AstDSL.intLiteral(123))),
            AstDSL.alias("123", AstDSL.intLiteral(123)),
-            AstDSL.alias("hello", AstDSL.stringLiteral("hello")),
+            AstDSL.alias("hello", stringLiteral("hello")),


I believe this was an accidental change done in a previous draft PR. Should be fixed under 57ec0691b36f1e0010c034df1a59ef6b364feaf4.

Yury-Fridlyand · 2022-07-27T00:13:18Z

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

@@ -694,7 +712,7 @@ public void parse_relation() {
            AstDSL.parse(
                AstDSL.relation("schema"),
                AstDSL.field("string_value"),
-                AstDSL.stringLiteral("(?<group>.*)")),
+                stringLiteral("(?<group>.*)")),


I believe this was an accidental change done in a previous draft PR. Should be fixed under 57ec0691b36f1e0010c034df1a59ef6b364feaf4.

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

sql/src/main/antlr/OpenSearchSQLParser.g4

Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto · 2022-07-27T23:11:08Z

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

+import org.opensearch.sql.planner.logical.LogicalPlan;
+
+@RequiredArgsConstructor
+public class HighlightAnalyzer  extends AbstractNodeVisitor<LogicalPlan, AnalysisContext> {


Class needs a javadoc

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

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java

acarbonetto · 2022-07-28T18:02:08Z

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java

+      refName += "." + StringUtils.unquoteText(getHighlightField().toString());
+    }
+    ExprValue retVal = valueEnv.resolve(DSL.ref(refName, ExprCoreType.STRING));
+    ImmutableMap.Builder<String, ExprValue> builder = new ImmutableMap.Builder<>();


You can declare this after the is statement.

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java

acarbonetto · 2022-07-28T18:04:36Z

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java

+      return retVal;
+    }
+
+    var hlBuilder = ImmutableMap.<String, ExprValue>builder();


mapBuilder or highlightMapBuilder?

Signed-off-by: forestmvey <forestv@bitquilltech.com>

…ing coverage for OpenSearchIndexScan Signed-off-by: forestmvey <forestv@bitquilltech.com>

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java

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

acarbonetto · 2022-07-29T16:28:25Z

core/src/test/java/org/opensearch/sql/planner/physical/HighlightOperatorTest.java

+public class HighlightOperatorTest extends PhysicalPlanTestBase {
+
+  @Test
+  public void highlight_all_test() {


what is this testing? that highlight all doesn't break anything?

I suppose this should be renamed to valid_highlight_operator_test. It is required for HighlightOperator coverage. The * isn't necessary for the highlight field.

sql/src/main/antlr/OpenSearchSQLParser.g4

sql/src/test/java/org/opensearch/sql/sql/antlr/HighlightTest.java

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

…light acceptance as a string literal as well as qualified name. Signed-off-by: forestmvey <forestv@bitquilltech.com>

MaxKsyunz · 2022-07-27T01:07:37Z

core/src/main/java/org/opensearch/sql/planner/physical/HighlightOperator.java

+public class HighlightOperator extends PhysicalPlan {
+  @NonNull
+  private final PhysicalPlan input;


In line with Peng's comment in the POC, this class ended up not doing anything and can be removed.

MitchellGale · 2022-07-29T20:01:37Z

@forestmvey looks like a test is failing now fyi.

    org.opentest4j.AssertionFailedError: expected: org.opensearch.sql.ast.expression.HighlightFunction@9443150<HighlightFunction(highlightField=fieldA)> but was: org.opensearch.sql.ast.expression.HighlightFunction@60bdba16<HighlightFunction(highlightField=fieldA)>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
        at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
        at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
        at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1124)
        at app//org.opensearch.sql.sql.parser.AstExpressionBuilderTest.canBuildHighlighFunction(AstExpressionBuilderTest.java:314)

MaxKsyunz

Once HighlightOperator goes away (or we can explain what it's for), other comments are addressed, and the CI tasks pass, I think it's good to go.

… PR. Signed-off-by: forestmvey <forestv@bitquilltech.com>

Signed-off-by: forestmvey <forestv@bitquilltech.com>

codecov · 2022-07-30T00:01:53Z

Codecov Report

Merging #96 (373353b) into integ_highlight-in-sql (5c6fd72) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##             integ_highlight-in-sql      #96      +/-   ##
============================================================
+ Coverage                     97.74%   97.77%   +0.02%     
- Complexity                     2858     2883      +25     
============================================================
  Files                           273      276       +3     
  Lines                          7020     7088      +68     
  Branches                        442      450       +8     
============================================================
+ Hits                           6862     6930      +68     
  Misses                          157      157              
  Partials                          1        1

Flag	Coverage Δ
sql-engine	`97.77% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...ensearch/sql/planner/physical/PhysicalPlanDSL.java	`100.00% <ø> (ø)`
.../sql/planner/physical/PhysicalPlanNodeVisitor.java	`100.00% <ø> (ø)`
...ain/java/org/opensearch/sql/analysis/Analyzer.java	`100.00% <100.00%> (ø)`
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <100.00%> (ø)`
...org/opensearch/sql/analysis/HighlightAnalyzer.java	`100.00% <100.00%> (ø)`
...ensearch/sql/expression/ExpressionNodeVisitor.java	`100.00% <100.00%> (ø)`
...opensearch/sql/expression/HighlightExpression.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/BuiltinFunctionName.java	`100.00% <100.00%> (ø)`
...h/sql/expression/function/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
...ensearch/sql/planner/logical/LogicalHighlight.java	`100.00% <100.00%> (ø)`
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto · 2022-07-30T03:27:54Z

Once HighlightOperator goes away (or we can explain what it's for), other comments are addressed, and the CI tasks pass, I think it's good to go.

At least, the two failing actions would be fixed by merging from upstream:main (opensearch-project#704) - I believe

Add XYPoint Indexer

MaxKsyunz and others added 20 commits July 26, 2022 09:29

Add support for highlight to parser and AstExpressionBuilder

1097d9f

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add unit test for highlight in AstExpressionBuilder

1a453e1

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add unit test for highlight in AstBuilderTest

0d5c87b

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Support highlight as an Unresolved expression.

26d0b7e

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Represent highlight as UnresolvedExpression.

3f10b8b

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Support highlight in Analyzer.

543d0d7

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Treat highlight as a proper function in AST

f47ffe7

In particular, highlightField is an expression now. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add support for highlight in Analyzer

5fdb939

HighlightFunction is converted to LogicalHighlight logical plan. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add a simple IT test for highlight.

5c8db0a

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Register highlight function in the BuiltInFunctionRepository

ac9f080

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Partial support for highlight in physical plan.

b526132

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Add HighlightOperator.

807c475

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Highlight alpha complete.

74b6492

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>

Initial implementation to upporting highlight('*')

ad7affc

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Add support for multiple highlight calls in select statement.

a192a21

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Cleaning up code, adding copyright, and adding javadocs

092d054

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Removed OpenSearchLogicalIndexScan highlightFields and dependencies.

9bdbb86

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Adding tests and touching up code in prep for PR.

9b463cf

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Fixing checkstyle errors.

2648e0c

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Added HighlightOperatorTest and additional testing.

7290e90

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Yury-Fridlyand reviewed Jul 27, 2022

View reviewed changes

integ-test/src/test/java/org/opensearch/sql/sql/HighlightFunctionIT.java Show resolved Hide resolved

Yury-Fridlyand reviewed Jul 27, 2022

View reviewed changes

sql/src/main/antlr/OpenSearchSQLParser.g4 Show resolved Hide resolved

Added HighlightExpressionTest

2feb2c4

Signed-off-by: forestmvey <forestv@bitquilltech.com>

forestmvey force-pushed the dev-highlight-in-sql branch from 9c777fc to 6c17cd7 Compare July 27, 2022 14:02

forestmvey requested a review from a team July 27, 2022 16:43

forestmvey force-pushed the dev-highlight-in-sql branch from 57ec069 to 7e1eab3 Compare July 27, 2022 21:49

acarbonetto reviewed Jul 27, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/ast/expression/HighlightFunction.java Show resolved Hide resolved

acarbonetto reviewed Jul 27, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java Show resolved Hide resolved

acarbonetto reviewed Jul 28, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java Show resolved Hide resolved

acarbonetto reviewed Jul 28, 2022

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java Show resolved Hide resolved

acarbonetto reviewed Jul 28, 2022

View reviewed changes

forestmvey added 2 commits July 28, 2022 16:05

Improving test coverage, fixing checkstyle errors, fixing jacoco errors.

cd6f911

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Added javadocs, minor PR revisions, and fixed jacoco errors by improv…

3db8788

…ing coverage for OpenSearchIndexScan Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto requested changes Jul 29, 2022

View reviewed changes

MitchellGale reviewed Jul 29, 2022

View reviewed changes

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java Show resolved Hide resolved

Code cleanup, adding parsing failure tests, and adding tests for high…

3ee3491

…light acceptance as a string literal as well as qualified name. Signed-off-by: forestmvey <forestv@bitquilltech.com>

forestmvey force-pushed the dev-highlight-in-sql branch from c0869c4 to 3ee3491 Compare July 29, 2022 18:51

MitchellGale approved these changes Jul 29, 2022

View reviewed changes

MaxKsyunz approved these changes Jul 29, 2022

View reviewed changes

MaxKsyunz self-requested a review July 29, 2022 20:01

MaxKsyunz requested changes Jul 29, 2022

View reviewed changes

forestmvey added 2 commits July 29, 2022 16:29

Removing HighlightOperator functionality and minor revisions based on…

e63b7b9

… PR. Signed-off-by: forestmvey <forestv@bitquilltech.com>

Removed unnecessary visitHighlight call in PhysicalPlanNodeVisitor

11d834b

Signed-off-by: forestmvey <forestv@bitquilltech.com>

Fixing formatting errors

373353b

Signed-off-by: forestmvey <forestv@bitquilltech.com>

acarbonetto approved these changes Jul 30, 2022

View reviewed changes

MaxKsyunz approved these changes Aug 1, 2022

View reviewed changes

forestmvey merged commit 1101db4 into integ_highlight-in-sql Aug 1, 2022

Yury-Fridlyand deleted the dev-highlight-in-sql branch January 12, 2023 19:46

andy-k-improving pushed a commit that referenced this pull request Nov 16, 2024

Merge pull request #96 from naveentatikonda/add_xy_point_indexer

1b149d5

Add XYPoint Indexer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Highlight In SQL #96

Add Highlight In SQL #96

forestmvey commented Jul 26, 2022

Yury-Fridlyand Jul 27, 2022

forestmvey Jul 27, 2022 •

edited

Loading

Yury-Fridlyand Jul 27, 2022

forestmvey Jul 27, 2022

Yury-Fridlyand Jul 27, 2022

MaxKsyunz Jul 27, 2022

forestmvey Jul 27, 2022

Yury-Fridlyand Jul 27, 2022

forestmvey Jul 27, 2022

Yury-Fridlyand Jul 27, 2022

forestmvey Jul 27, 2022

acarbonetto Jul 27, 2022

acarbonetto Jul 28, 2022

acarbonetto Jul 28, 2022

acarbonetto Jul 29, 2022

forestmvey Jul 29, 2022 •

edited

Loading

MaxKsyunz Jul 27, 2022

MitchellGale commented Jul 29, 2022

MaxKsyunz left a comment •

edited

Loading

codecov bot commented Jul 30, 2022 •

edited

Loading

acarbonetto commented Jul 30, 2022

Add Highlight In SQL #96

Add Highlight In SQL #96

Conversation

forestmvey commented Jul 26, 2022

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

forestmvey Jul 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

forestmvey Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchellGale commented Jul 29, 2022

MaxKsyunz left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2022 • edited Loading

Codecov Report

acarbonetto commented Jul 30, 2022

forestmvey Jul 27, 2022 •

edited

Loading

forestmvey Jul 29, 2022 •

edited

Loading

MaxKsyunz left a comment •

edited

Loading

codecov bot commented Jul 30, 2022 •

edited

Loading