fix : added support for multiple values for CONTAIN, START_WITH and END_WITH operators #11068
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
product
![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
Outside diff range, codebase verification and nitpick comments (3)
metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java (3)
149-209: Add assertions for null and empty values.The test method should also include assertions for null and empty values to ensure robustness.
+ // Add assertions for null and empty values + final Criterion nullValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.CONTAIN).setValue(null); + result = ESUtils.getQueryBuilderFromCriterion(nullValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"**\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected); + + final Criterion emptyValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.CONTAIN).setValue(""); + result = ESUtils.getQueryBuilderFromCriterion(emptyValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"**\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected);
212-273: Add assertions for null and empty values.The test method should also include assertions for null and empty values to ensure robustness.
+ // Add assertions for null and empty values + final Criterion nullValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.START_WITH).setValue(null); + result = ESUtils.getQueryBuilderFromCriterion(nullValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"*\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected); + + final Criterion emptyValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.START_WITH).setValue(""); + result = ESUtils.getQueryBuilderFromCriterion(emptyValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"*\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected);
276-336: Add assertions for null and empty values.The test method should also include assertions for null and empty values to ensure robustness.
+ // Add assertions for null and empty values + final Criterion nullValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.END_WITH).setValue(null); + result = ESUtils.getQueryBuilderFromCriterion(nullValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"*\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected); + + final Criterion emptyValueCriterion = new Criterion().setField("myTestField").setCondition(Condition.END_WITH).setValue(""); + result = ESUtils.getQueryBuilderFromCriterion(emptyValueCriterion, false, new HashMap<>(), mock(AspectRetriever.class)); + expected = "{\n" + " \"wildcard\" : {\n" + " \"myTestField.keyword\" : {\n" + " \"wildcard\" : \"*\",\n" + " \"boost\" : 1.0,\n" + " \"_name\" : \"myTestField\"\n" + " }\n" + " }\n" + "}"; + Assert.assertEquals(result.toString(), expected);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java (1 hunks)
- metadata-io/src/test/java/com/linkedin/metadata/search/utils/ESUtilsTest.java (1 hunks)
Additional comments not posted (5)
metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java (5)
592-603: LGTM!The method correctly constructs a wildcard query for a single value.
605-616: LGTM!The method correctly constructs a wildcard query for the CONTAIN condition.
618-629: LGTM!The method correctly constructs a wildcard query for the START_WITH condition.
631-642: LGTM!The method correctly constructs a wildcard query for the END_WITH condition.
561-568: Ensure consistent query naming.The method correctly delegates to specific methods for handling different conditions. Ensure that the query naming is consistent across all conditions.
Verification successful
Query naming is consistent across all conditions.
The
queryNameparameter is consistently passed and used in the methodsbuildContainsConditionFromCriterion,buildStartsWithConditionFromCriterion, andbuildEndsWithConditionFromCriterion. No issues were found regarding query naming consistency.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the query naming is consistent across all conditions. # Test: Search for queryName usage. Expect: Consistent query naming. rg --type java 'queryName'Length of output: 7903
Script:
#!/bin/bash # Search for the definitions of the methods to verify the usage of `queryName`. # Find the definition of `buildContainsConditionFromCriterion` ast-grep --lang java --pattern 'buildContainsConditionFromCriterion($_, $_, $_, $_, $_)' # Find the definition of `buildStartsWithConditionFromCriterion` ast-grep --lang java --pattern 'buildStartsWithConditionFromCriterion($_, $_, $_, $_, $_)' # Find the definition of `buildEndsWithConditionFromCriterion` ast-grep --lang java --pattern 'buildEndsWithConditionFromCriterion($_, $_, $_, $_, $_)'Length of output: 1143
Script:
#!/bin/bash # Search for the definitions of the methods to verify the usage of `queryName`. # Find the definition of `buildContainsConditionFromCriterion` ast-grep --lang java --pattern 'public $_ buildContainsConditionFromCriterion($_, $_, $_, $_, $_)' # Find the definition of `buildStartsWithConditionFromCriterion` ast-grep --lang java --pattern 'public $_ buildStartsWithConditionFromCriterion($_, $_, $_, $_, $_)' # Find the definition of `buildEndsWithConditionFromCriterion` ast-grep --lang java --pattern 'public $_ buildEndsWithConditionFromCriterion($_, $_, $_, $_, $_)'Length of output: 1770
Script:
#!/bin/bash # Search for the definitions of the methods to verify the usage of `queryName`. # Find the definition of `buildContainsConditionFromCriterion` ast-grep --lang java --pattern 'function buildContainsConditionFromCriterion($_, $_, $_, $_, $_) { $$$ }' # Find the definition of `buildStartsWithConditionFromCriterion` ast-grep --lang java --pattern 'function buildStartsWithConditionFromCriterion($_, $_, $_, $_, $_) { $$$ }' # Find the definition of `buildEndsWithConditionFromCriterion` ast-grep --lang java --pattern 'function buildEndsWithConditionFromCriterion($_, $_, $_, $_, $_) { $$$ }'Length of output: 1812
Script:
#!/bin/bash # Search for the definitions of the methods to verify the usage of `queryName`. # Find the definition of `buildContainsConditionFromCriterion` rg 'buildContainsConditionFromCriterion' -A 10 metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java # Find the definition of `buildStartsWithConditionFromCriterion` rg 'buildStartsWithConditionFromCriterion' -A 10 metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java # Find the definition of `buildEndsWithConditionFromCriterion` rg 'buildEndsWithConditionFromCriterion' -A 10 metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.javaLength of output: 3632
| private static QueryBuilder buildWildcardQueryWithMultipleValues( | ||
| @Nonnull final String fieldName, | ||
| @Nonnull final Criterion criterion, | ||
| final boolean isTimeseries, | ||
| @Nullable String queryName, | ||
| @Nonnull AspectRetriever aspectRetriever, | ||
| String wildcardPattern) { | ||
| BoolQueryBuilder boolQuery = QueryBuilders.boolQuery(); | ||
|
|
||
| for (String value : criterion.getValues()) { | ||
| boolQuery.should(QueryBuilders.wildcardQuery( | ||
| toKeywordField(criterion.getField(), isTimeseries, aspectRetriever), | ||
| String.format(wildcardPattern, ESUtils.escapeReservedCharacters(value.trim()))) | ||
| .queryName(queryName != null ? queryName : fieldName)); | ||
| } | ||
| return boolQuery; | ||
| } |
There was a problem hiding this comment.
Optimize wildcard query construction.
The method can be optimized by using a stream to construct the wildcard query.
- BoolQueryBuilder boolQuery = QueryBuilders.boolQuery();
- for (String value : criterion.getValues()) {
- boolQuery.should(QueryBuilders.wildcardQuery(
- toKeywordField(criterion.getField(), isTimeseries, aspectRetriever),
- String.format(wildcardPattern, ESUtils.escapeReservedCharacters(value.trim())))
- .queryName(queryName != null ? queryName : fieldName));
- }
- return boolQuery;
+ return criterion.getValues().stream()
+ .map(value -> QueryBuilders.wildcardQuery(
+ toKeywordField(criterion.getField(), isTimeseries, aspectRetriever),
+ String.format(wildcardPattern, ESUtils.escapeReservedCharacters(value.trim())))
+ .queryName(queryName != null ? queryName : fieldName))
+ .collect(Collectors.collectingAndThen(Collectors.toList(), queries -> {
+ BoolQueryBuilder boolQuery = QueryBuilders.boolQuery();
+ queries.forEach(boolQuery::should);
+ return boolQuery;
+ }));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static QueryBuilder buildWildcardQueryWithMultipleValues( | |
| @Nonnull final String fieldName, | |
| @Nonnull final Criterion criterion, | |
| final boolean isTimeseries, | |
| @Nullable String queryName, | |
| @Nonnull AspectRetriever aspectRetriever, | |
| String wildcardPattern) { | |
| BoolQueryBuilder boolQuery = QueryBuilders.boolQuery(); | |
| for (String value : criterion.getValues()) { | |
| boolQuery.should(QueryBuilders.wildcardQuery( | |
| toKeywordField(criterion.getField(), isTimeseries, aspectRetriever), | |
| String.format(wildcardPattern, ESUtils.escapeReservedCharacters(value.trim()))) | |
| .queryName(queryName != null ? queryName : fieldName)); | |
| } | |
| return boolQuery; | |
| } | |
| private static QueryBuilder buildWildcardQueryWithMultipleValues( | |
| @Nonnull final String fieldName, | |
| @Nonnull final Criterion criterion, | |
| final boolean isTimeseries, | |
| @Nullable String queryName, | |
| @Nonnull AspectRetriever aspectRetriever, | |
| String wildcardPattern) { | |
| return criterion.getValues().stream() | |
| .map(value -> QueryBuilders.wildcardQuery( | |
| toKeywordField(criterion.getField(), isTimeseries, aspectRetriever), | |
| String.format(wildcardPattern, ESUtils.escapeReservedCharacters(value.trim()))) | |
| .queryName(queryName != null ? queryName : fieldName)) | |
| .collect(Collectors.collectingAndThen(Collectors.toList(), queries -> { | |
| BoolQueryBuilder boolQuery = QueryBuilders.boolQuery(); | |
| queries.forEach(boolQuery::should); | |
| return boolQuery; | |
| })); | |
| } |
|
@david-leifker can you please review the above PR? |
|
@Nbagga14 - Please run |
Nbagga14
force-pushed
the
support-multiple-values
branch
from
5864d2f to
174e928
Compare
|
Thank you! |
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests