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.

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

Move mappers to separate files #448

Merged

Conversation

martin-gaievski
Copy link
Member

Signed-off-by: Martin Gaievski gaievski@amazon.com

Description

This is in preparation for new LuceneField mapper to keep size of KNNVectorFieldMapper manageable and to structure project better.
Extract mapper classes from file with abstract class KNNVectorFieldMapper to separate files, created new "mapper" package for new classes.

Issues Resolved

building block for #39

Check List

  • All tests pass
  • Commits are signed as 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.

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
@martin-gaievski martin-gaievski added Refactoring Improve the design, structure, and implementation while preserving its functionality 2.2.0 backport 2.x labels Jul 14, 2022
@martin-gaievski martin-gaievski requested a review from a team July 14, 2022 17:30
@martin-gaievski martin-gaievski self-assigned this Jul 14, 2022
}
}

Optional<float[]> getFloatsFromContext(ParseContext context, int dimension) throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

This part will be common with Lucene field mapper, extract it to a method to avoid code duplication

if (!KNNSettings.isKNNPluginEnabled()) {
throw new IllegalStateException("KNN plugin is disabled. To enable " + "update knn.plugin.enabled setting to true");
validateIfKNNPluginEnabled();
validateIfCircuitBreakerTriggered();
Copy link
Member Author

Choose a reason for hiding this comment

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

This part will be common with Lucene field mapper, extract it to a method to avoid code duplication

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #448 (efd487e) into main (7499375) will increase coverage by 0.03%.
The diff coverage is 86.90%.

@@             Coverage Diff              @@
##               main     #448      +/-   ##
============================================
+ Coverage     83.82%   83.86%   +0.03%     
- Complexity      933      948      +15     
============================================
  Files           133      136       +3     
  Lines          3828     3842      +14     
  Branches        349      349              
============================================
+ Hits           3209     3222      +13     
- Misses          457      458       +1     
  Partials        162      162              
Impacted Files Coverage Δ
.../main/java/org/opensearch/knn/index/IndexUtil.java 55.73% <ø> (ø)
...n/java/org/opensearch/knn/index/KNNIndexShard.java 93.75% <ø> (ø)
...java/org/opensearch/knn/index/KNNQueryBuilder.java 67.96% <ø> (ø)
...index/codec/KNN80Codec/KNN80DocValuesConsumer.java 88.67% <ø> (ø)
...java/org/opensearch/knn/indices/ModelMetadata.java 97.08% <ø> (ø)
...main/java/org/opensearch/knn/plugin/KNNPlugin.java 100.00% <ø> (ø)
.../opensearch/knn/plugin/script/KNNScoringSpace.java 91.66% <ø> (ø)
...nsearch/knn/plugin/script/KNNScoringSpaceUtil.java 96.55% <ø> (ø)
.../opensearch/knn/index/mapper/ModelFieldMapper.java 71.42% <71.42%> (ø)
...nsearch/knn/index/mapper/KNNVectorFieldMapper.java 81.81% <79.16%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7499375...efd487e. Read the comment docs.

if (!KNNSettings.isKNNPluginEnabled()) {
throw new IllegalStateException("KNN plugin is disabled. To enable " + "update knn.plugin.enabled setting to true");
validateIfKNNPluginEnabled();
validateIfCircuitBreakerTriggered();
Copy link
Member

Choose a reason for hiding this comment

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

nit: May be validateIfCircuitBreakerIsNotTriggered??

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@@ -69,7 +61,7 @@
*/
public abstract class KNNVectorFieldMapper extends ParametrizedFieldMapper {

private static Logger logger = LogManager.getLogger(KNNVectorFieldMapper.class);
static Logger logger = LogManager.getLogger(KNNVectorFieldMapper.class);
Copy link
Member

Choose a reason for hiding this comment

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

lombok?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

Copy link
Member Author

Choose a reason for hiding this comment

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

realized that lombok logger is only private. we used that old logger in one of the sub-classes, I guess that's ok to create it's own logger specifically for that class. It will change a log file content a bit, as I think it places class name in the line prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

make it protected rather than package private, why not create the logger in the class which is using it rather than creating it in the parent class.

Comment on lines 3 to 9
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@@ -69,7 +61,7 @@
*/
public abstract class KNNVectorFieldMapper extends ParametrizedFieldMapper {

private static Logger logger = LogManager.getLogger(KNNVectorFieldMapper.class);
static Logger logger = LogManager.getLogger(KNNVectorFieldMapper.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not private?

Copy link
Member Author

@martin-gaievski martin-gaievski Jul 14, 2022

Choose a reason for hiding this comment

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

I was thinking or re-using same logger in sub-class to keep content of the log file same, as it puts the class name there.
But I on second though it should be a problem as long as we keep the content same.
Let me update, I also will use lombok for logger initialization

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest use different loggers for different classes specially for parent and child as sometimes it become difficult to find the right code path.

Comment on lines 53 to 61
"Model \""
+ modelId
+ "\" from "
+ context.mapperService().index().getName()
+ "'s mapping does not exist. Because the "
+ "\""
+ MODEL_ID
+ "\" parameter is not updateable, this index will need to "
+ "be recreated with a valid model."
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use String.format here, rather than "+" operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

…license header

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
VijayanB
VijayanB previously approved these changes Jul 14, 2022

void validateIfKNNPluginEnabled() {
if (!KNNSettings.isKNNPluginEnabled()) {
throw new IllegalStateException("KNN plugin is disabled. To enable " + "update knn.plugin.enabled setting to true");
Copy link
Member

Choose a reason for hiding this comment

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

nit: Remove + in middle?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

import static org.opensearch.knn.common.KNNConstants.SPACE_TYPE;

/**
* Field mapper for original implementation
Copy link
Member

Choose a reason for hiding this comment

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

minor: Do you think we should add what the mapping looks like so users have more context?

{
   "type": "knn_vector",
   "dimension": 128
}

Also, add something like "This is the mapper for the original version. It defaults to using nmslib as the engine and retrieves parameters from index settings."

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

String spaceType = indexSettings.get(KNNSettings.INDEX_KNN_SPACE_TYPE.getKey());
if (spaceType == null) {
log.info(
"[KNN] The setting \""
Copy link
Member

Choose a reason for hiding this comment

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

Should this use string builder?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I think build and/or String.format for params, ack

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
@martin-gaievski martin-gaievski merged commit cfac544 into opensearch-project:main Jul 14, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 14, 2022
* Move mappers to separate files

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
(cherry picked from commit cfac544)
martin-gaievski pushed a commit that referenced this pull request Jul 21, 2022
* Move mappers to separate files

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
@heemin32 heemin32 added v2.2.0 and removed 2.2.0 labels Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Refactoring Improve the design, structure, and implementation while preserving its functionality v2.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants