Add Highlight Support in PPL and Wildcard in SQL and PPL #110

forestmvey · 2022-08-23T16:07:16Z

Description

Support the highlight function in PPL. Add support for wildcard in SQL and PPL.

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 · 2022-08-23T16:25:07Z

Codecov Report

Merging #110 (43b841d) into integ-highlight-in-ppl (8a7b329) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##             integ-highlight-in-ppl     #110      +/-   ##
============================================================
+ Coverage                     94.85%   94.88%   +0.02%     
- Complexity                     2910     2925      +15     
============================================================
  Files                           288      288              
  Lines                          7830     7875      +45     
  Branches                        570      575       +5     
============================================================
+ Hits                           7427     7472      +45     
  Misses                          349      349              
  Partials                         54       54

Flag	Coverage Δ
query-workbench	`62.76% <ø> (ø)`
sql-engine	`97.81% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ensearch/sql/expression/ExpressionNodeVisitor.java	`100.00% <ø> (ø)`
...h/sql/expression/function/OpenSearchFunctions.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%> (ø)`
...search/sql/planner/physical/HighlightOperator.java	`100.00% <100.00%> (ø)`
.../sql/planner/physical/PhysicalPlanNodeVisitor.java	`100.00% <100.00%> (ø)`
...ecutor/protector/OpenSearchExecutionProtector.java	`100.00% <100.00%> (ø)`
...search/sql/opensearch/storage/OpenSearchIndex.java	`100.00% <100.00%> (ø)`
...java/org/opensearch/sql/ppl/parser/AstBuilder.java	`100.00% <100.00%> (ø)`
...pensearch/sql/ppl/parser/AstExpressionBuilder.java	`100.00% <100.00%> (ø)`
... and 1 more

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

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

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

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

docs/user/ppl/functions/relevance.rst

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

MaxKsyunz · 2022-09-02T21:01:16Z

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

+    // In the event of multiple returned highlights and wildcard being
+    // used in conjunction with other highlight calls, we need to ensure
+    // only wildcard regex matching is mapped to wildcard call.
+    if (StringUtils.unquoteText(highlight.toString()).matches("(.+\\*)|(\\*.+)")


Is this regex only matches strings begining or ending with *?
I.e., will it match B*o*d*y?

It will not match B*o*d*y, yes it must start or end with a star. As well in your example opensearch would not return any value so this if statement would fail the and section of this if statement.

I'd like to double-check that. From looking at the code, OpenSearch does support * in the middle of the expression.

Highlight eventually uses HighlightPhase, which uses OpenSearch's Regex class which supports something like B*o*d*y. It was not the best example, but I think B*dy will match.

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

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

MaxKsyunz · 2022-09-02T22:19:45Z

@forestmvey I'd like to have a second look at Object.toString usages.

I understand that in context it provides required data, but semantically toString is intended for something else. This is what the docs have to say about it.

forestmvey · 2022-09-02T22:31:03Z

Can you please be more specific and give an example of what you would like revised.

MaxKsyunz · 2022-09-06T10:12:50Z

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

+  private Pair<String, ExprValue> mapHighlight(Environment<Expression, ExprValue> env) {
+    String osHighlightKey = "_highlight";
+    if (!highlight.toString().contains("*")) {
+      osHighlightKey += "." + StringUtils.unquoteText(highlight.toString());


StringUtils.unquoteText(highlight.toString()) is the same as highlight.valueOf(null).string().
It is also used here several times and can be calculated once and stored in a variable.

MaxKsyunz · 2022-09-06T10:13:53Z

Can you please be more specific and give an example of what you would like revised.

This is the part I was thinking of.

MaxKsyunz · 2022-09-06T10:18:37Z

Also, based on this comment, it'd be better if HighlightOperator moves to :opensearch module.

That should also allow us to use OpenSearch's Regex class and make pattern matching consistent.

…t in SQL and PPL. Signed-off-by: forestmvey <forestv@bitquilltech.com>

MaxKsyunz · 2022-09-07T23:01:54Z

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

+    // In the event of multiple returned highlights and wildcard being
+    // used in conjunction with other highlight calls, we need to ensure
+    // only wildcard regex matching is mapped to wildcard call.
+    if (highlightStr.contains("*") && value.type() == STRUCT) {
+      value = new ExprTupleValue(
+          new LinkedHashMap<String, ExprValue>(value.tupleValue()
+              .entrySet()
+              .stream()
+              .filter(s -> matchesHighlightRegex(s.getKey(), highlightStr))
+              .collect(Collectors.toMap(
+                  e -> e.getKey(),
+                  e -> e.getValue()))));
+    }


🌟 I like the comment and the use of filter.

forestmvey requested a review from a team August 23, 2022 16:49

forestmvey marked this pull request as ready for review August 23, 2022 16:53

MaxKsyunz requested changes Aug 26, 2022

View reviewed changes

MaxKsyunz requested review from MitchellGale, Yury-Fridlyand and acarbonetto August 29, 2022 16:57

forestmvey force-pushed the dev-highlight-in-ppl branch from 000537d to ccc29e3 Compare September 1, 2022 15:34

forestmvey changed the title ~~Add Highlight Support in PPL~~ Add Highlight Support in PPL and Wildcard in SQL and PPL Sep 1, 2022

MitchellGale approved these changes Sep 1, 2022

View reviewed changes

MaxKsyunz requested review from MaxKsyunz and MitchellGale September 1, 2022 16:56

Yury-Fridlyand reviewed Sep 2, 2022

View reviewed changes

docs/user/ppl/functions/relevance.rst Show resolved Hide resolved

Yury-Fridlyand reviewed Sep 2, 2022

View reviewed changes

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

MitchellGale approved these changes Sep 2, 2022

View reviewed changes

Yury-Fridlyand approved these changes Sep 2, 2022

View reviewed changes

MaxKsyunz requested changes Sep 2, 2022

View reviewed changes

forestmvey force-pushed the dev-highlight-in-ppl branch from ccc29e3 to 4fa96cf Compare September 2, 2022 22:34

MaxKsyunz reviewed Sep 6, 2022

View reviewed changes

MaxKsyunz self-requested a review September 6, 2022 19:27

forestmvey force-pushed the dev-highlight-in-ppl branch 3 times, most recently from 458bb34 to ca08438 Compare September 7, 2022 16:03

Adding Highlight support in PPL. Adding wildcard support for highligh…

43b841d

…t in SQL and PPL. Signed-off-by: forestmvey <forestv@bitquilltech.com>

forestmvey force-pushed the dev-highlight-in-ppl branch from ca08438 to 43b841d Compare September 7, 2022 16:25

MaxKsyunz approved these changes Sep 8, 2022

View reviewed changes

forestmvey merged commit 74dfcf3 into integ-highlight-in-ppl Sep 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Highlight Support in PPL and Wildcard in SQL and PPL #110

Add Highlight Support in PPL and Wildcard in SQL and PPL #110

forestmvey commented Aug 23, 2022 •

edited

Loading

codecov bot commented Aug 23, 2022 •

edited

Loading

MaxKsyunz Sep 2, 2022

forestmvey Sep 2, 2022

MaxKsyunz Sep 6, 2022

MaxKsyunz commented Sep 2, 2022

forestmvey commented Sep 2, 2022

MaxKsyunz Sep 6, 2022

MaxKsyunz commented Sep 6, 2022 •

edited

Loading

MaxKsyunz commented Sep 6, 2022

MaxKsyunz Sep 7, 2022

Add Highlight Support in PPL and Wildcard in SQL and PPL #110

Add Highlight Support in PPL and Wildcard in SQL and PPL #110

Conversation

forestmvey commented Aug 23, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Aug 23, 2022 • edited Loading

Codecov Report

MaxKsyunz Sep 2, 2022

Choose a reason for hiding this comment

forestmvey Sep 2, 2022

Choose a reason for hiding this comment

MaxKsyunz Sep 6, 2022

Choose a reason for hiding this comment

MaxKsyunz commented Sep 2, 2022

forestmvey commented Sep 2, 2022

MaxKsyunz Sep 6, 2022

Choose a reason for hiding this comment

MaxKsyunz commented Sep 6, 2022 • edited Loading

MaxKsyunz commented Sep 6, 2022

MaxKsyunz Sep 7, 2022

Choose a reason for hiding this comment

forestmvey commented Aug 23, 2022 •

edited

Loading

codecov bot commented Aug 23, 2022 •

edited

Loading

MaxKsyunz commented Sep 6, 2022 •

edited

Loading