Refactor OpenSearch Relevance Functions out of Core #2019

MitchellGale · 2023-08-22T23:54:50Z

Description

Moving Relevance-based search functions out of core module and into opensearch module.
This only affects the Function Resolver.

Out of Scope:

OpenSearchFunction resolver was not moved, as the score tracking is needed in the analyzer and optimizer.
Parser
Analyzer and its sub-components
LogicalPlan or optimizer
Design for refactoring the analyzer and creating an OpenSearch analyzer: Added design for core refactor Bit-Quill/opensearch-project-sql#331

Issues Resolved

Partially fixes #811

Internal review: Bit-Quill#338

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.

* Created Relevance and OpenSeach Functions in opensearch module Signed-off-by: Guian Gumpac <[email protected]> * Moving Test in progress. Committing for rebase purposes Signed-off-by: Guian Gumpac <[email protected]> * Moved tests out from core Signed-off-by: Guian Gumpac <[email protected]> * Pulled out analyzers to opensearch module Signed-off-by: Guian Gumpac <[email protected]> * Revert "Pulled out analyzers to opensearch module" This reverts commit a602e30. * Fixed jacoco for opensearch module Signed-off-by: Guian Gumpac <[email protected]> * Refactor out OpenSearchFunction from OpenSearchFunctions; Move OpenSearchFunctions and RelevanceFunctionResolver Signed-off-by: acarbonetto <[email protected]> * Fixed tests in core. WIP Signed-off-by: Guian Gumpac <[email protected]> * Update to use OpenSearchDSL Signed-off-by: acarbonetto <[email protected]> * Fixed nested test coverage Signed-off-by: Guian Gumpac <[email protected]> * Update datasource metadata to check for initial cluster state Signed-off-by: acarbonetto <[email protected]> * Add score functions to analyzer test Signed-off-by: acarbonetto <[email protected]> * Re-add nested tests into core Signed-off-by: acarbonetto <[email protected]> * Added hashcode for DataSourceMetadata Signed-off-by: Guian Gumpac <[email protected]> * Added test for score without boost Signed-off-by: Guian Gumpac <[email protected]> * Re-add nested tests in Analyzer Signed-off-by: acarbonetto <[email protected]> * Clean up analyzer tests Signed-off-by: acarbonetto <[email protected]> * Cleaned up some code Signed-off-by: Guian Gumpac <[email protected]> * Fix jacoco and add unit tests for coverage Signed-off-by: acarbonetto <[email protected]> --------- Signed-off-by: Guian Gumpac <[email protected]> Signed-off-by: acarbonetto <[email protected]> Co-authored-by: acarbonetto <[email protected]>

…vance Signed-off-by: acarbonetto <[email protected]>

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

codecov · 2023-08-23T00:26:51Z

Codecov Report

Merging #2019 (f008573) into main (ed2b683) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2019   +/-   ##
=========================================
  Coverage     97.30%   97.30%           
- Complexity     4623     4626    +3     
=========================================
  Files           407      409    +2     
  Lines         11935    11946   +11     
  Branches        828      830    +2     
=========================================
+ Hits          11613    11624   +11     
  Misses          315      315           
  Partials          7        7

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

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

Files Changed	Coverage Δ
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java	`100.00% <100.00%> (ø)`
...rch/sql/analysis/ExpressionReferenceOptimizer.java	`100.00% <100.00%> (ø)`
...c/main/java/org/opensearch/sql/expression/DSL.java	`100.00% <100.00%> (ø)`
...expression/function/BuiltinFunctionRepository.java	`100.00% <100.00%> (ø)`
...ql/expression/function/NestedFunctionResolver.java	`100.00% <100.00%> (ø)`
...ch/sql/expression/function/OpenSearchFunction.java	`100.00% <100.00%> (ø)`
...s/storage/OpenSearchDataSourceMetadataStorage.java	`98.77% <100.00%> (+0.01%)`	⬆️
.../sql/opensearch/functions/OpenSearchFunctions.java	`100.00% <100.00%> (ø)`
...pensearch/functions/RelevanceFunctionResolver.java	`100.00% <100.00%> (ø)`
...ql/opensearch/storage/OpenSearchStorageEngine.java	`100.00% <100.00%> (ø)`
... and 1 more

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

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

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

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

core/src/test/java/org/opensearch/sql/expression/function/DefaultFunctionResolverTest.java

acarbonetto · 2023-08-23T00:47:29Z

core/src/test/java/org/opensearch/sql/expression/function/OpenSearchFunctionTest.java

+    assertEquals("match(test=\"test\")", function.toString());
+  }
+
+  //  @Test


acarbonetto · 2023-08-23T00:49:35Z

.../src/test/java/org/opensearch/sql/opensearch/planner/optimizer/LogicalPlanOptimizerTest.java

+    lenient().when(table.createScanBuilder()).thenReturn(tableScanBuilder);
+  }
+
+  //  @Test


remove this file... it provides no new tests

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

core/src/test/java/org/opensearch/sql/expression/ExpressionTestBase.java

opensearch/src/test/java/org/opensearch/sql/opensearch/config/TestConfig.java

opensearch/src/test/java/org/opensearch/sql/opensearch/expression/ExpressionTestBase.java

opensearch/src/test/java/org/opensearch/sql/opensearch/planner/DefaultImplementorTest.java

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

opensearch/src/test/java/org/opensearch/sql/opensearch/config/TestConfig.java

opensearch/src/test/java/org/opensearch/sql/opensearch/expression/ExpressionTestBase.java

Yury-Fridlyand · 2023-08-23T04:31:41Z

core/src/main/java/org/opensearch/sql/datasource/model/EmptyDataSourceService.java

+import org.opensearch.sql.storage.StorageEngine;
+import org.opensearch.sql.storage.Table;
+
+public class EmptyDataSourceService {


Need a javadoc and a good description why it is here

Yury-Fridlyand · 2023-08-23T04:32:42Z

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

@@ -46,7 +49,7 @@ public class BuiltinFunctionRepository {
  private final Map<FunctionName, FunctionResolver> functionResolverMap;

  /** The singleton instance. */
-  private static BuiltinFunctionRepository instance;
+  private static final Map<Integer, BuiltinFunctionRepository> instance = new HashMap<>();


Extend the comment and probably rename to instances.

Yury-Fridlyand · 2023-08-23T04:34:15Z

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

+    Set<DataSourceMetadata> dataSourceMetadataSet = dataSourceService.getDataSourceMetadata(true);
+    Set<Integer> dataSourceServiceHashSet =
+        dataSourceMetadataSet.stream()
+            .map(metadata -> metadata.hashCode())


I think it could be optimized to

Suggested change

.map(metadata -> metadata.hashCode())

.map(Object::hashCode)

Yury-Fridlyand · 2023-08-23T04:34:46Z

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

+            .getDataSource(metadata.getName())
+            .getStorageEngine()
+            .getFunctions()
+            .forEach(function -> repository.register(function));


Suggested change

.forEach(function -> repository.register(function));

.forEach(repository::register);

Yury-Fridlyand · 2023-08-23T04:35:36Z

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

+        instance.put(metadata.hashCode(), repository);
+      }


Suggested change

instance.put(metadata.hashCode(), repository);

}

}

instance.put(metadata.hashCode(), repository);

Yury-Fridlyand · 2023-08-23T04:41:29Z

core/src/test/java/org/opensearch/sql/expression/function/OpenSearchFunctionTest.java

+                new NamedArgumentExpression(
+                    "test", new LiteralExpression(new ExprStringValue("test")))));
+    assertEquals(BOOLEAN, function.type());
+    assertThrows(UnsupportedOperationException.class, () -> function.valueOf(null));


Check exception message

Yury-Fridlyand · 2023-08-23T04:41:44Z

...rc/main/java/org/opensearch/sql/datasources/storage/OpenSearchDataSourceMetadataStorage.java

@@ -80,6 +80,9 @@ public OpenSearchDataSourceMetadataStorage(

  @Override
  public List<DataSourceMetadata> getDataSourceMetadata() {
+    if (!this.clusterService.getClusterApplierService().isInitialClusterStateSet()) {


Add a comment when this happens

Yury-Fridlyand · 2023-08-23T04:42:53Z

opensearch/src/test/java/org/opensearch/sql/opensearch/config/TestConfig.java

+  public static final String INT_TYPE_NULL_VALUE_FIELD = "int_null_value";
+  public static final String INT_TYPE_MISSING_VALUE_FIELD = "int_missing_value";
+  public static final String DOUBLE_TYPE_NULL_VALUE_FIELD = "double_null_value";
+  public static final String DOUBLE_TYPE_MISSING_VALUE_FIELD = "double_missing_value";


Are all these things used in the test?

Yury-Fridlyand · 2023-08-23T04:44:07Z

opensearch/src/test/java/org/opensearch/sql/opensearch/expression/OpenSearchDSL.java

+public class OpenSearchDSL {
+  private OpenSearchDSL() {}
+
+  public static LiteralExpression literal(Byte value) {


No need to copy these things from DSL: literal, ref and named

Yury-Fridlyand · 2023-08-23T04:45:22Z

opensearch/src/test/java/org/opensearch/sql/opensearch/planner/DefaultImplementorTest.java

+
+    LogicalPlan plan =
+        project(
+            nested(


No need to copy the entire set. Keep project, nested and values

MitchellGale requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners August 22, 2023 23:54

acarbonetto added 2 commits August 22, 2023 17:06

Merge remote-tracking branch 'upstream/main' into integ-refactor-rele…

8296478

…vance Signed-off-by: acarbonetto <[email protected]>

SpotlessApply

b7f3b24

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

acarbonetto changed the title ~~Refactor Relevance out of Core~~ Refactor OpenSearch Relevance Functions out of Core Aug 23, 2023

MitchellGale commented Aug 23, 2023

View reviewed changes

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

acarbonetto reviewed Aug 23, 2023

View reviewed changes

acarbonetto added 2 commits August 22, 2023 17:58

Clean up tests and javadoc

e4fa5fc

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

Remove commented out tests

0fb46fc

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

acarbonetto reviewed Aug 23, 2023

View reviewed changes

core/src/test/java/org/opensearch/sql/expression/ExpressionTestBase.java Show resolved Hide resolved

acarbonetto reviewed Aug 23, 2023

View reviewed changes

opensearch/src/test/java/org/opensearch/sql/opensearch/config/TestConfig.java Show resolved Hide resolved

acarbonetto reviewed Aug 23, 2023

View reviewed changes

opensearch/src/test/java/org/opensearch/sql/opensearch/expression/ExpressionTestBase.java Show resolved Hide resolved

acarbonetto reviewed Aug 23, 2023

View reviewed changes

opensearch/src/test/java/org/opensearch/sql/opensearch/planner/DefaultImplementorTest.java Show resolved Hide resolved

acarbonetto previously approved these changes Aug 23, 2023

View reviewed changes

Clean up and spotlessApply

f008573

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

acarbonetto dismissed their stale review via f008573 August 23, 2023 01:33

Yury-Fridlyand mentioned this pull request Aug 23, 2023

PoC Refactor: Move relevance search functions from :core to :opensearch #2025

Draft

6 tasks

acarbonetto approved these changes Aug 23, 2023

View reviewed changes

opensearch/src/test/java/org/opensearch/sql/opensearch/config/TestConfig.java Show resolved Hide resolved

opensearch/src/test/java/org/opensearch/sql/opensearch/expression/ExpressionTestBase.java Show resolved Hide resolved

Yury-Fridlyand reviewed Aug 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor OpenSearch Relevance Functions out of Core #2019

Refactor OpenSearch Relevance Functions out of Core #2019

MitchellGale commented Aug 22, 2023 •

edited by acarbonetto

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

acarbonetto Aug 23, 2023

acarbonetto Aug 23, 2023

acarbonetto Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

Yury-Fridlyand Aug 23, 2023

	.forEach(function -> repository.register(function));
	.forEach(repository::register);

Refactor OpenSearch Relevance Functions out of Core #2019

Are you sure you want to change the base?

Refactor OpenSearch Relevance Functions out of Core #2019

Conversation

MitchellGale commented Aug 22, 2023 • edited by acarbonetto Loading

Description

Issues Resolved

Check List

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

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

MitchellGale commented Aug 22, 2023 •

edited by acarbonetto

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading