Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Highlight In SQL #717

Merged
merged 24 commits into from
Aug 5, 2022
Merged

Add Highlight In SQL #717

merged 24 commits into from
Aug 5, 2022

Conversation

forestmvey
Copy link
Collaborator

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.

MaxKsyunz and others added 20 commits July 26, 2022 09:29
@forestmvey
Add support for highlight to parser and AstExpressionBuilder
1097d9f 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Add unit test for highlight in AstExpressionBuilder
1a453e1 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Add unit test for highlight in AstBuilderTest
0d5c87b 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Support highlight as an Unresolved expression.
26d0b7e 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Represent highlight as UnresolvedExpression.
3f10b8b 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Support highlight in Analyzer.
543d0d7 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Treat highlight as a proper function in AST
f47ffe7 
In particular, highlightField is an expression now.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Add support for highlight in Analyzer
5fdb939 
HighlightFunction is converted to LogicalHighlight logical plan.

Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Add a simple IT test for highlight.
5c8db0a 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Register highlight function in the BuiltInFunctionRepository
ac9f080 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Partial support for highlight in physical plan.
b526132 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Add HighlightOperator.
807c475 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Highlight alpha complete.
74b6492 
Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
@forestmvey
Initial implementation to upporting highlight('*')
ad7affc 
Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Add support for multiple highlight calls in select statement.
bb97dde 
Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Removed OpenSearchLogicalIndexScan highlightFields and dependencies. …
163b909 
…Improved test coverage and fixing checkstyle errors.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Added HighlightExpressionTest
b2a90d4 
Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Added javadocs, minor PR revisions, and fixed jacoco errors by improv…
2c04c49 
…ing coverage for OpenSearchIndexScan

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Code cleanup, adding parsing failure tests, and adding tests for high…
01708e3 
…light acceptance as a string literal as well as qualified name.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Removing HighlightOperator functionality and unnecessary visitHighlig…
6b96ac0 
…ht call in PhysicalPlanNodeVisitor..

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey forestmvey requested a review from a team as a code owner August 1, 2022 17:48
dai-chen
dai-chen reviewed Aug 2, 2022
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering are we sending another PR for user manual separately?

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/expression/HighlightExpression.java Show resolved Hide resolved
@forestmvey
Adding highlight function to functions.rst and removing unecessary fu…
232b19f 
…nction call in OpenSearchIndexScan.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Copy link
Collaborator Author

forestmvey commented Aug 3, 2022 •
edited

I'm wondering are we sending another PR for user manual separately?

I added highlight to functions.rst under commit: 232b19f5924d2c46fd08b9d60abadb0d207ad016. Documentation website will be handled in a separate PR.

@forestmvey
Change highlight fields returned format to array list. Changed highli…
9fbea30 
…ght all and wildcard to unsupported to open up output formatting changes for multiple returned highlight fields. Change tests for updated coverage and disable highlight all and wildcard tests.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Copy link
Collaborator Author

Under commit 9fbea3072bbf6cd3f89851a20344eecf720ac219 I have changed the output format to be more useable to the end user. See revisions to functions.rst for new output format. I have also changed highlight all, and wildcard to unsupported for this version as we want to iterate on the output format for multiple returned highlight fields.

dai-chen
dai-chen previously approved these changes Aug 4, 2022
View reviewed changes
acarbonetto
acarbonetto previously approved these changes Aug 4, 2022
View reviewed changes
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@forestmvey
Fix bug where invalid schema name was being used for returned highlig…
23a24e2 
…ht fields

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@forestmvey
Fix failing integration tests due to schema changes for highlight exp…
449a330 
…ression type.

Signed-off-by: forestmvey <forestv@bitquilltech.com>
@penghuo penghuo merged commit 8f1b31d into opensearch-project:main Aug 5, 2022
@MaxKsyunz MaxKsyunz added enhancement New feature or request SQL full-text labels Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxKsyunz MaxKsyunz MaxKsyunz requested changes

@anirudha anirudha anirudha approved these changes

@dai-chen dai-chen dai-chen approved these changes

@acarbonetto acarbonetto acarbonetto left review comments

Assignees
No one assigned
Labels
enhancement New feature or request SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@forestmvey @anirudha @dai-chen @MaxKsyunz @acarbonetto @penghuo