Skip to content

Conversation

@MitchellGale
Copy link

Signed-off-by: Mitchell Gale [email protected]

Description

Allows for any case on argument name for relevancy based function (Match Phrase Prefix, Simple Query String, Match Bool Prefix, Match, Multi-Match, Query String, Relevance, Simple Query String).
e.g.
AnalyZer can work instead of just analyzer
Zero_Terms_Query can work instead of just zero_terms_query
BOOst can work instead of just boost

Allows for zero_term_query arguments to be of any case.
e.g.
All can work instead of all

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.

while (iterator.hasNext()) {
NamedArgumentExpression arg = (NamedArgumentExpression) iterator.next();
if (!queryBuildActions.containsKey(arg.getArgName())) {
String argNormalized = arg.getArgName().toLowerCase();

Choose a reason for hiding this comment

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

The isLowerCase may fail if arg.getArgName() doesn't exist.

Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

SQL/PPL parser is already case-insensitive. The goal is to change case for some parameter values.
See example from main:

opensearchsql> select * from logs where simple_query_string(['*'], 'CLR', AnAlYZer = 'standard');
Output longer than terminal width
Do you want to display data vertically for better visual effect? [y/N]: y
opensearchsql> select * from logs where simple_query_string(['*'], 'CLR', analyzer = 'stAndaRd');
TransportError(500, 'SearchPhaseExecutionException', {'error': {'type': 'SearchPhaseExecutionException', 'reason': 'Error occurred in OpenSearch engine: all shards failed', 'details': 'Shard[0]: [logs/uHvAufvyREut9LnosEX2yg] QueryShardException[[simple_query_string] analyzer [stAndaRd] not found]\n\nFor more details, please send request for Json format to see the raw response from OpenSearch engine.'}, 'status': 503})

So all calls for analyzer's value should be wrapped by .toLower():

.put("analyzer", (b, v) -> b.analyzer(v.stringValue()))

like you did for ZeroTermsQuery and Flags. Boost has double type, nothing to convert there, but what about Operator, DefaultOperator and so on?

@MaxKsyunz
Copy link

@MitchellGale-BitQuill the conflicts need to be resolved and SQL Java CI should pass as well.

MitchellGale and others added 4 commits August 2, 2022 14:23
… is surrounded by same type of quote (opensearch-project#696)

Signed-off-by: mitchellg <[email protected]>

Co-authored-by: Andrew Carbonetto <[email protected]>
* Add support for highlight to parser and AstExpressionBuilder

Signed-off-by: MaxKsyunz <[email protected]>

* Add unit test for highlight in AstExpressionBuilder

Signed-off-by: MaxKsyunz <[email protected]>

* Add unit test for highlight in AstBuilderTest

Signed-off-by: MaxKsyunz <[email protected]>

* Support highlight as an Unresolved expression.

Signed-off-by: MaxKsyunz <[email protected]>

* Represent highlight as UnresolvedExpression.

Signed-off-by: MaxKsyunz <[email protected]>

* Support highlight in Analyzer.

Signed-off-by: MaxKsyunz <[email protected]>

* Treat highlight as a proper function in AST

In particular, highlightField is an expression now.

Signed-off-by: MaxKsyunz <[email protected]>

* Add support for highlight in Analyzer

HighlightFunction is converted to LogicalHighlight logical plan.

Signed-off-by: MaxKsyunz <[email protected]>

* Add a simple IT test for highlight.

Signed-off-by: MaxKsyunz <[email protected]>

* Register highlight function in the BuiltInFunctionRepository

Signed-off-by: MaxKsyunz <[email protected]>

* Partial support for highlight in physical plan.

Signed-off-by: MaxKsyunz <[email protected]>

* Add HighlightOperator.

Signed-off-by: MaxKsyunz <[email protected]>

* Highlight alpha complete.

Signed-off-by: MaxKsyunz <[email protected]>

* Initial implementation to upporting highlight('*')

Signed-off-by: forestmvey <[email protected]>

* Add support for multiple highlight calls in select statement.

Signed-off-by: forestmvey <[email protected]>

* Removed OpenSearchLogicalIndexScan highlightFields and dependencies. Improved test coverage and fixing checkstyle errors.

Signed-off-by: forestmvey <[email protected]>

* Added HighlightExpressionTest

Signed-off-by: forestmvey <[email protected]>

* Added javadocs, minor PR revisions, and fixed jacoco errors by improving coverage for OpenSearchIndexScan

Signed-off-by: forestmvey <[email protected]>

* Code cleanup, adding parsing failure tests, and adding tests for highlight acceptance as a string literal as well as qualified name.

Signed-off-by: forestmvey <[email protected]>

* Removing HighlightOperator functionality and unnecessary visitHighlight call in PhysicalPlanNodeVisitor..

Signed-off-by: forestmvey <[email protected]>

* Adding highlight function to functions.rst and removing unecessary function call in OpenSearchIndexScan.

Signed-off-by: forestmvey <[email protected]>

* Change highlight fields returned format to array list. Changed highlight 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 <[email protected]>

* Fix bug where invalid schema name was being used for returned highlight fields

Signed-off-by: forestmvey <[email protected]>

* Fix failing integration tests due to schema changes for highlight expression type.

Signed-off-by: forestmvey <[email protected]>

Co-authored-by: MaxKsyunz <[email protected]>
@MitchellGale MitchellGale force-pushed the dev-RelFunc_changeCaseWhereAppropriate branch from 534aa05 to 145b0c5 Compare August 5, 2022 17:57
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #95 (8c49d05) into Integ-RelFunc_changeCaseWhereAppropriate (5c6fd72) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                              Coverage Diff                               @@
##             Integ-RelFunc_changeCaseWhereAppropriate      #95      +/-   ##
==============================================================================
+ Coverage                                       97.74%   97.77%   +0.02%     
- Complexity                                       2858     2882      +24     
==============================================================================
  Files                                             273      276       +3     
  Lines                                            7020     7086      +66     
  Branches                                          442      447       +5     
==============================================================================
+ Hits                                             6862     6928      +66     
  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 14 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@MitchellGale MitchellGale force-pushed the dev-RelFunc_changeCaseWhereAppropriate branch from 145b0c5 to 6b2802a Compare August 5, 2022 18:43
Signed-off-by: mitchellg <[email protected]>
Signed-off-by: mitchellg <[email protected]>
Signed-off-by: mitchellg <[email protected]>
Signed-off-by: mitchellg <[email protected]>
Signed-off-by: mitchellg <[email protected]>
Signed-off-by: mitchellg <[email protected]>
@MitchellGale
Copy link
Author

#103 PR remade in 103

@Yury-Fridlyand Yury-Fridlyand deleted the dev-RelFunc_changeCaseWhereAppropriate branch January 12, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants