Allow different data types for category in Context suggester#23491
Allow different data types for category in Context suggester#23491rjernst merged 5 commits intoelastic:masterfrom
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
@areek Request you to look at this fix. |
|
Hi Requesting some one from Elasticsearch to verify this fix. |
rjernst
left a comment
There was a problem hiding this comment.
Sorry for the long delay. I left some comments. This also needs tests.
| final Set<CharSequence> contexts = new HashSet<>(); | ||
| Token token = parser.currentToken(); | ||
| if (token == Token.VALUE_STRING) { | ||
| if (token == Token.VALUE_NULL) { |
There was a problem hiding this comment.
I don't think we should be adding support for null here. It should remain an error.
| if (token == Token.VALUE_STRING) { | ||
| if (token == Token.VALUE_STRING || token == Token.VALUE_NUMBER || token == Token.VALUE_BOOLEAN) { | ||
| contexts.add(parser.text()); | ||
| } else if (token == Token.VALUE_NULL) { |
There was a problem hiding this comment.
Ditto here; this should not be added.
|
@rjernst Thanks for the review, I have incorporated the changes as suggested by you. |
rjernst
left a comment
There was a problem hiding this comment.
Thank you for the tests and changes! One more request for the tests, and could you also please update the PR title and description to explain what is changing.
| .endArray() | ||
| .endObject() | ||
| .bytes()); | ||
| } catch(MapperParsingException e) { |
There was a problem hiding this comment.
You can use expectThrows(MapperParsingException.class, () -> { /* code that throws */});, then check the message on the returned exception.
|
@elasticmachine please test this |
|
@rjernst as suggested all changes are done. Please verify. |
|
Thanks @nilabhsagar |
The "category" in context suggester could be String, Number or Boolean. However with the changes in version 5 this is failing and only accepting String. This will have problem for existing users of Elasticsearch if they choose to migrate to higher version; as their existing Mapping and query will fail as mentioned in a bug #22358 This PR fixes the above mentioned issue and allows user to migrate seamlessly. Closes #22358
* master: (41 commits) Remove awaits fix from evil JNA native tests Correct handling of default and array settings Build: Switch jna dependency to an elastic version (elastic#24081) fix CategoryContextMappingTests compilation bugs testConcurrentGetAndSetOnPrimary - fix a race condition between indexing and updating value map Allow different data types for category in Context suggester (elastic#23491) Restrict build info loading to ES jar, not any jar (elastic#24049) Remove more hidden file leniency from plugins Register error listener in evil logger tests Detect using logging before configuration [DOCS] Added note about Elastic Cloud to improve 'elastic aws' SERP results. Add version constant for 5.5 (elastic#24075) Add unit tests for NestedAggregator (elastic#24054) Add more debugging information to rethrottles Tests: Use random analyzer only on string fields in Match/MultiMatchBuilderTests Cleanup outdated comments for fixing up pom dependencies (elastic#24056) S3 Repository: Eagerly load static settings (elastic#23910) Reject duplicate settings on the command line Wildcard cluster names for cross cluster search (elastic#23985) Update scripts/security docs for sandboxed world (elastic#23977) ...
|
Thanks @rjernst |
The "category" in context suggester could be String, Number or Boolean. However with the changes in version 5 this is failing and only accepting String. This will have problem for existing users of Elasticsearch if they choose to migrate to higher version; as their existing Mapping and query will fail as mentioned in a bug #22358
This PR fixes the above mentioned issue and allows user to migrate seamlessly.
Note: Providing the NULL value for a category at index and query time will throw an exception.
Closes #22358