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

Correct search_after handling #50629

Merged

Conversation

aleksmaus
Copy link
Member

Correct the initial "naive" handling of search_after parameters.

@aleksmaus aleksmaus requested review from costin and imotov January 4, 2020 13:31
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Good catch! Left some suggestions.

// TODO: finalize the endpoints
controller.registerHandler(GET, "/{index}/_eql/search", this);
controller.registerHandler(POST, "/{index}/_eql/search", this);
final String path = "/{index}/_eql/search";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make a constant out of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

for (int i = 0; i < size; i++) {
arr.add(randomAlphaOfLength(randomIntBetween(1, 15)));
private SearchAfterBuilder randomJsonSearchFromBuilder() throws IOException {
int numSearchAfter = randomIntBetween(1, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about an empty list?

Copy link
Member Author

Choose a reason for hiding this comment

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

normally SearchAfter doesn't accept the empty list, throws error:

    java.lang.IllegalArgumentException: Values must contains at least one value.

The original searchafter test doesn't use empty list as well

Copy link
Contributor

Choose a reason for hiding this comment

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

How about returning setting search after values to null sometimes then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Already handled there as well

    java.lang.NullPointerException: Values cannot be null.
        at __randomizedtesting.SeedInfo.seed([1C63F70E75AEE77B:48BB964652FCDAE7]:0)
        at org.elasticsearch.search.searchafter.SearchAfterBuilder.setSortValues(SearchAfterBuilder.java:78)

Copy link
Member Author

Choose a reason for hiding this comment

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

discussed over slack, updated implementation

jsonBuilder.startObject();
jsonBuilder.startArray("search_after");
for (int i = 0; i < numSearchAfter; i++) {
int branch = randomInt(9);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is LuceneTests.randomSortValue() that generates random sort values. I wonder if we could use it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not possible, I think there is a better pattern for this base on randomFrom(...) and Supplier<...>

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to a better pattern

@aleksmaus aleksmaus force-pushed the fix/eql_search_after_handling branch from 1717177 to 68ee15f Compare January 6, 2020 20:07
@aleksmaus aleksmaus force-pushed the fix/eql_search_after_handling branch from 68ee15f to 1bd4327 Compare January 6, 2020 20:31
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@aleksmaus aleksmaus merged commit 3777324 into elastic:feature/eql Jan 6, 2020
@aleksmaus aleksmaus deleted the fix/eql_search_after_handling branch January 6, 2020 22:57
aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request Jan 27, 2020
@aleksmaus aleksmaus mentioned this pull request Jan 27, 2020
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