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

fix: better exception message #969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: better exception message #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions src/main/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,26 +162,41 @@ public void rescoreSearchResponse(
*/
public void byFieldSearchHitValidator(final SearchHit hit) {
if (!hit.hasSource()) {
log.error(String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%d]", hit.docId()));
log.error(String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%s]", hit.getId()));
throw new IllegalArgumentException(
String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%d]", hit.docId())
String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%s]", hit.getId())
);
}

Map<String, Object> sourceMap = hit.getSourceAsMap();
if (!mappingExistsInSource(sourceMap, targetField)) {
log.error(String.format(Locale.ROOT, "The field to rerank [%s] is not found at hit [%d]", targetField, hit.docId()));
log.error(String.format(Locale.ROOT, "The field to rerank [%s] is not found at hit [%s]", targetField, hit.getId()));

throw new IllegalArgumentException(String.format(Locale.ROOT, "The field to rerank by is not found at hit [%d]", hit.docId()));
throw new IllegalArgumentException(String.format(Locale.ROOT, "The field to rerank by is not found at hit [%s]", hit.getId()));
}

Optional<Object> val = getValueFromSource(sourceMap, targetField);

if (!(val.get() instanceof Number)) {
log.error(String.format(Locale.ROOT, "The field mapping to rerank [%s: %s] is not Numerical", targetField, val.orElse(null)));
// Strictly get the type of value removing the prefix of getClass() having a value is guaranteed so no NPE check
String typeOfMapping = val.get().getClass().toString().replace("class ", "");
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that val.get().getClass().toString() also have strings like class ? I would suggest replaceFirst.

Copy link
Member

Choose a reason for hiding this comment

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

getClass().getSimpleName() should be enough to avoid the replace and give you just the class name

log.error(
String.format(
Locale.ROOT,
"The field mapping to rerank [%s: %s] is not Numerical, instead of type [%s]",
targetField,
val.orElse(null),
typeOfMapping
)
);

throw new IllegalArgumentException(
String.format(Locale.ROOT, "The field mapping to rerank by [%s] is not Numerical", val.orElse(null))
String.format(
Locale.ROOT,
"The field mapping to rerank by [%s] is not Numerical, instead of type [%s]",
val.orElse(null),
typeOfMapping
)
);
}

Expand Down
11 changes: 7 additions & 4 deletions src/test/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ public void testRerank_throwsExceptionOnNoSource_WhenSearchResponseHasNoSourceMa
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(listener, times(1)).onFailure(argumentCaptor.capture());

assertEquals("There is no source field to be able to perform rerank on hit [" + 1 + "]", argumentCaptor.getValue().getMessage());
assertEquals("There is no source field to be able to perform rerank on hit [" + 2 + "]", argumentCaptor.getValue().getMessage());
assert (argumentCaptor.getValue() instanceof IllegalArgumentException);
}

Expand Down Expand Up @@ -907,7 +907,7 @@ public void testRerank_throwsExceptionOnMappingNotExistingInSource_WhenSearchRes
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(listener, times(1)).onFailure(argumentCaptor.capture());

assertEquals("The field to rerank by is not found at hit [" + 1 + "]", argumentCaptor.getValue().getMessage());
assertEquals("The field to rerank by is not found at hit [" + 2 + "]", argumentCaptor.getValue().getMessage());
assert (argumentCaptor.getValue() instanceof IllegalArgumentException);
}

Expand Down Expand Up @@ -947,7 +947,7 @@ public void testRerank_throwsExceptionOnHavingEmptyMapping_WhenTargetFieldHasNul
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(listener, times(1)).onFailure(argumentCaptor.capture());

assertEquals("The field to rerank by is not found at hit [" + 1 + "]", argumentCaptor.getValue().getMessage());
assertEquals("The field to rerank by is not found at hit [" + 2 + "]", argumentCaptor.getValue().getMessage());
assert (argumentCaptor.getValue() instanceof IllegalArgumentException);
}

Expand Down Expand Up @@ -985,7 +985,10 @@ public void testRerank_throwsExceptionOnHavingNonNumericValue_WhenTargetFieldHas
ArgumentCaptor<Exception> argumentCaptor = ArgumentCaptor.forClass(Exception.class);
verify(listener, times(1)).onFailure(argumentCaptor.capture());

assertEquals("The field mapping to rerank by [hello world] is not Numerical", argumentCaptor.getValue().getMessage());
assertEquals(
"The field mapping to rerank by [hello world] is not Numerical, instead of type [java.lang.String]",
Copy link
Member

Choose a reason for hiding this comment

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

Can you turn it into a string format, where [java.lang.String] gets in the same way as typeOfMapping?

argumentCaptor.getValue().getMessage()
);
assert (argumentCaptor.getValue() instanceof IllegalArgumentException);

}
Expand Down
Loading