Skip to content

Search in multivalue fields - #6

Merged
e-kolpakov merged 1 commit into
masterfrom
ekolpakov/multivalue-search
Mar 31, 2015
Merged

Search in multivalue fields#6
e-kolpakov merged 1 commit into
masterfrom
ekolpakov/multivalue-search

Conversation

@e-kolpakov

Copy link
Copy Markdown
Contributor

Description: Documented search behavior of using contains predicate instead of equal_to if search value is singular and document value is multivalue one.
Background: the only supported search engine so far (elastic) analyses multivalues fields essentially resulting in "contains in" search using term filter/query, as opposed to "equals to" when non-multivalue field is used. This behavior considered a good syntax, so it was documented and covered with tests.
JIRA ticket: SOL-591

Also, I've took the responsibility of splitting single 1500+ line long test module into smaller modules. There are no changes in moved test suites except necessary modifications to patch calls.

@e-kolpakov
e-kolpakov force-pushed the ekolpakov/multivalue-search branch 3 times, most recently from 99c0ed1 to dbd93e5 Compare March 27, 2015 13:40
@e-kolpakov
e-kolpakov force-pushed the ekolpakov/multivalue-search branch 2 times, most recently from 7e0ff82 to a712f86 Compare March 27, 2015 14:17
… to" predicate when searching in multivalue fields
@e-kolpakov
e-kolpakov force-pushed the ekolpakov/multivalue-search branch from a712f86 to b5444b2 Compare March 27, 2015 14:26
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling b5444b2 on ekolpakov/multivalue-search into 21ac6b0 on master.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@e-kolpakov Are these lyrics under a compatible license? I know it may seem silly, but it's something we probably need to be sure of.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@e-kolpakov Nevermind, I see this existed beforehand.

@Kelketek

Copy link
Copy Markdown

@e-kolpakov Aside from that question about the docs, this looks good to me. 👍

@e-kolpakov

Copy link
Copy Markdown
Contributor Author

@martynjames you might want to review this (or assign someone else)

@martynjames

Copy link
Copy Markdown

Thanks @e-kolpakov - will look at it. Would've been easier if the separating of the tests into different files would have been separated from the actual changes you were implementing. Never mind about that now - but this is one case where having 2 different commits would actually make sense!

@martynjames

Copy link
Copy Markdown

👍 - Please next time consider separating the refactoring into a separate PR or at least a separate commit.

@e-kolpakov

Copy link
Copy Markdown
Contributor Author

@martynjames sure, thank you for review.

e-kolpakov added a commit that referenced this pull request Mar 31, 2015
@e-kolpakov
e-kolpakov merged commit aa76dda into master Mar 31, 2015
@e-kolpakov
e-kolpakov deleted the ekolpakov/multivalue-search branch March 31, 2015 13:22
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.

4 participants