Add mappings for enrich fields (#96056)#10
Add mappings for enrich fields (#96056)#10MitchLewis930 wants to merge 1 commit intopr_020_beforefrom
Conversation
We are developing a new component that performs lookup during query time. The idea is to utilize the existing enrich policies and indices used during indexing. However, to ensure proper functionality of the new component, we need the mapping types and doc_values of the enrich fields. With this change, populating the mapping for enrich fields is a best-effort process to maintain the current behavior. This means that any mapping conflicts arising between the enrich fields of the source indices will be ignored. I will follow up to address this validation issue.
📝 WalkthroughWalkthroughAdds a changelog entry and refactors enrich mapping handling in EnrichPolicyRunner to use a MappingTypeAndFormat record and centralized createEnrichMapping logic; expands tests with conflict-mapping cases and adds an assertEnrichMapping helper (duplicated in the file). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java`:
- Around line 228-238: The error message in EnrichPolicyRunner's formats
conflict branch uses policy.getMatchField() which is incorrect for non-match
fields; replace that usage with the local fieldName variable so the exception
shows the actual field being validated (update the thrown ElasticsearchException
message invocation in the if (formats.size() > 1) block to pass fieldName
instead of policy.getMatchField(), keeping the rest of the message/arguments the
same).
🧹 Nitpick comments (1)
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java (1)
300-302: Unreachable null check (dead code).This null check is unreachable. If
currentFieldwere null after the loop, the method would have already returned at Line 290. This appears to be leftover from refactoring.♻️ Suggested cleanup
- if (currentField == null) { - return null; - } final String type = (String) currentField.getOrDefault("type", "object");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/changelog/96056.yamlx-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.javax-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java
🔇 Additional comments (10)
x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java (4)
42-44: LGTM!The new imports (
Maps,Iterables,HashMap) are appropriately added to support the new map-driven mapping construction logic.Also applies to: 60-60
308-340: LGTM!The
RANGE_TYPESset correctly includes all range types (integer_range,float_range,long_range,double_range,ip_range,date_range), and themappingForMatchFieldmethod properly handles all policy types with appropriate error handling for unknown types.
342-398: LGTM!The
createEnrichMappingmethod correctly handles the case where the match field is also an enrich field by enablingdoc_values(removing the explicitfalsesetting). The map-driven approach is clean and the XContentBuilder usage for serialization is appropriate.
400-418: LGTM!The integration of the new
createEnrichMapping(mappings)method intoprepareAndCreateEnrichIndexis clean and maintains the existing control flow.docs/changelog/96056.yaml (1)
1-5: LGTM!The changelog entry correctly documents this enhancement with appropriate PR number, summary, area, and type.
x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java (5)
35-35: LGTM!The new imports (
PlainActionFuture,XContentHelper,ElasticsearchAssertions,Locale) are properly added to support the new test functionality.Also applies to: 42-42, 58-58, 77-77
2305-2310: LGTM!The
assertEnrichMappinghelper method is well-designed, validating thatdynamicis set to"false"and comparing thepropertiesagainst the expected JSON mapping.
2085-2141: LGTM!This test correctly validates the behavior when enrich fields have conflicting mapping types across source indices. It verifies that:
- Fields with conflicting types (
name) are excluded from the explicit enrich mapping- Documents still contain the original data regardless of mapping conflicts
- Fields with consistent types (
user,zipcode) are properly mapped
2143-2185: LGTM!This test correctly validates the behavior when enrich fields have conflicting date formats across source indices. It confirms that the
datefield is excluded from the explicit mapping due to format conflict while documents retain their original data.
160-175: LGTM!The refactoring of mapping assertions to use the declarative
assertEnrichMappinghelper with JSON strings improves test readability and consistency across all test methods.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (formats.size() > 1) { | ||
| if (strictlyRequired) { | ||
| throw new ElasticsearchException( | ||
| "Multiple distinct formats specified for field '{}' - indices({}) format entries({})", | ||
| policy.getMatchField(), | ||
| Strings.collectionToCommaDelimitedString(policy.getIndices()), | ||
| Strings.collectionToCommaDelimitedString(formats) | ||
| ); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect field name in error message for format conflicts.
Line 232 uses policy.getMatchField() in the error message, but this method can be called for any field (including enrich fields). This is inconsistent with the type conflict error at Line 217 which correctly uses fieldName.
🐛 Proposed fix
if (formats.size() > 1) {
if (strictlyRequired) {
throw new ElasticsearchException(
"Multiple distinct formats specified for field '{}' - indices({}) format entries({})",
- policy.getMatchField(),
+ fieldName,
Strings.collectionToCommaDelimitedString(policy.getIndices()),
Strings.collectionToCommaDelimitedString(formats)
);
}🤖 Prompt for AI Agents
In
`@x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunner.java`
around lines 228 - 238, The error message in EnrichPolicyRunner's formats
conflict branch uses policy.getMatchField() which is incorrect for non-match
fields; replace that usage with the local fieldName variable so the exception
shows the actual field being validated (update the thrown ElasticsearchException
message invocation in the if (formats.size() > 1) block to pass fieldName
instead of policy.getMatchField(), keeping the rest of the message/arguments the
same).
2 participants
PR_020
Summary by CodeRabbit
Enhancements
Tests
Documentation