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

Adding test cases for Non object + object mapping error #24

Open
wants to merge 6 commits into
base: airbnb-main
Choose a base branch
from

Conversation

zarna1parekh
Copy link
Collaborator

@zarna1parekh zarna1parekh commented Feb 14, 2025

Summary

Adding test cases for Non object + object mapping error

To test out the error seen on index node:
Error doing map update errorMsg=can't merge a non object mapping [alerts] with an object mapping

The error is seen when the same parent field is being used as object and non-object. When this happens, the aggregation operation fails on for the parent object. The document will still be inserted.

Currently, we are not doing very complex aggregation. So as far as we are not dropping the document, we are okay to proceed so moving the log level also to warning for the error message.

Requirements

Copy link

@baroquebobcat baroquebobcat left a comment

Choose a reason for hiding this comment

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

Looks pretty good.
I have a couple general questions, a number of style suggestions and one nitpick about a stale comment.

@@ -546,7 +546,7 @@ private static boolean tryRegisterField(
new CompressedXContent(BytesReference.bytes(mapping)),
MapperService.MergeReason.MAPPING_UPDATE);
} catch (Exception e) {
LOG.error("Error doing map update errorMsg={}", e.getMessage());
LOG.warn("Error doing map update errorMsg={}", e.getMessage());

Choose a reason for hiding this comment

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

I think it would be good if this error message highlighted

  • That this is for mappings used by aggregations
  • What field is impacted

It could also pass the exception again as a dangling argument so the stacktrace and class are reported.

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.

2 participants