DRAFT - Add support for highlight in PPL #107

forestmvey · 2022-08-17T23:03:59Z

Description
Add PPL support for highlight in SQL plugin.

Issues Resolved
Issue: github-636

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-17T23:11:57Z

Codecov Report

Merging #107 (c656026) into draft-highlight-in-ppl (ce15448) will increase coverage by 0.02%.
The diff coverage is 100.00%.

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

@@                     Coverage Diff                      @@
##             draft-highlight-in-ppl     #107      +/-   ##
============================================================
+ Coverage                     94.85%   94.88%   +0.02%     
- Complexity                     2913     2926      +13     
============================================================
  Files                           288      289       +1     
  Lines                          7839     7879      +40     
  Branches                        571      573       +2     
============================================================
+ Hits                           7436     7476      +40     
  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 Δ
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <ø> (ø)`
...ensearch/storage/script/core/ExpressionScript.java	`100.00% <ø> (ø)`
...pensearch/sql/protocol/response/format/Format.java	`100.00% <ø> (ø)`
...ain/java/org/opensearch/sql/analysis/Analyzer.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%> (ø)`

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

* Bump checkstyle version and fix violations Signed-off-by: Chen Dai <daichen@amazon.com> * Fix checkstyle violations in test code Signed-off-by: Chen Dai <daichen@amazon.com> Signed-off-by: Chen Dai <daichen@amazon.com>

Yury-Fridlyand · 2022-08-18T00:12:13Z

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

+   */
+  @Override
+  public LogicalPlan visitHighlight(Highlight node, AnalysisContext context) {
+    LogicalPlan child = node.getChild().get(0).accept(this, context);


Just to confirm that .get(0) won't crash if there is no such element.

I don't believe this case would be possible. As well the returning object at index 0 is standard for visit functions in the Analyzer.

Yury-Fridlyand · 2022-08-18T00:16:15Z

core/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java

+  public T visitHighlightFunction(HighlightFunction node, C context) {
+    return visitChildren(node, context);
+  }
+
+  public T visitHighlight(Highlight node, C context) {
    return visitChildren(node, context);
  }


What is the difference?

visitHighlightFunction takes a HighlightFunction node and has an overriding function in ExpressionAnalyzer, while visitHighlight takes a highlight node and has an overriding function in Analyzer. This is because for PPL I was unable to resolve a highlight field as an expression like I did in SQL. Instead I use HighlightOperator and use the Analyzer to get around this issue. This issue arose because of the way the parser was implemented, the function highlight syntax of not having highlight in a fields command, and the parser implementation of AllFields without a fields command.

We may want to consider updating SQL to use the highlightOperator rather than HighlightExpression...

MaxKsyunz · 2022-08-18T01:45:26Z

@forestmvey I didn't realize you recreated the PR.

Please have a look at my comments to the previous PR here.

MitchellGale · 2022-08-19T07:16:19Z

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

@@ -79,4 +79,8 @@ public R visitMLCommons(PhysicalPlan node, C context) {
  public R visitAD(PhysicalPlan node, C context) {
    return visitNode(node, context);
  }
+
+  public R visitHighlight(PhysicalPlan node, C context) {


What's the difference between visitHighlight and visitAD?

AD and highlight operators need their own visiting functions to be called from their derived classes. You can find the overriding functions in OpenSearchExecutionProtector. The nodes passed into these functions will be for their respective HighlightOperator and ADOperator. This way the operator nodes are passed in, we can successfully visit the input for each node.

MitchellGale · 2022-08-19T07:19:17Z

ppl/src/test/java/org/opensearch/sql/ppl/parser/AstBuilderTest.java

@@ -607,6 +609,26 @@ public void testParseCommand() {
        ));
  }

+  @Test
+  public void testStringLiteralHighlightCommand() {
+    assertEqual("source=t | highlight('FieldA')",


can double quotes be used in the highlight parameter? If so, should it be tested?

Yes double quotes in highlight parameter are supported. Both single and double quoted parameters will be interpreted as string literals at this parsing level. I don't think it's overly necessary to add a test for it at this level.

MitchellGale · 2022-08-19T07:19:51Z

ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java

@@ -25,7 +25,6 @@
 import static org.opensearch.sql.ast.dsl.AstDSL.exprList;
 import static org.opensearch.sql.ast.dsl.AstDSL.field;
 import static org.opensearch.sql.ast.dsl.AstDSL.filter;
-import static org.opensearch.sql.ast.dsl.AstDSL.floatLiteral;


Was this just an unused import?

Yes this was just an un-used import.

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>