-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
SQL: clear the cursor if nested inner hits are enough to fulfill the query required limits #35398
Changes from 10 commits
274ca56
be11569
741fa59
8865f11
d216bf2
2da8b1d
934383f
2a0851c
a87fc1a
f067c56
0aae5af
9f7588a
3b120e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,14 @@ public class JdbcCsvSpecIT extends CsvSpecTestCase { | |
public JdbcCsvSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase) { | ||
super(fileName, groupName, testName, lineNumber, testCase); | ||
} | ||
|
||
@Override | ||
protected int fetchSize() { | ||
// using a smaller fetchSize for nested documents' tests to uncover bugs | ||
// similar with https://github.com/elastic/elasticsearch/issues/35176 quicker | ||
if (fileName.startsWith("nested")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of overriding protected int fetchSize() {
if (fileName.startsWith("nested")) {
return randomBoolean() ? super.fetchSize() : randomIntBetween(1,5));
}
} |
||
return randomBoolean() ? super.fetchSize() : randomIntBetween(1, 5); | ||
} | ||
return super.fetchSize(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,19 +318,20 @@ protected void handleResponse(SearchResponse response, ActionListener<SchemaRowS | |
// there are some results | ||
if (hits.length > 0) { | ||
String scrollId = response.getScrollId(); | ||
|
||
SchemaSearchHitRowSet hitRowSet = new SchemaSearchHitRowSet(schema, exts, hits, query.limit(), scrollId); | ||
|
||
// if there's an id, try to setup next scroll | ||
if (scrollId != null && | ||
// is all the content already retrieved? | ||
(Boolean.TRUE.equals(response.isTerminatedEarly()) || response.getHits().getTotalHits() == hits.length | ||
// or maybe the limit has been reached | ||
|| (hits.length >= query.limit() && query.limit() > -1))) { | ||
(Boolean.TRUE.equals(response.isTerminatedEarly()) | ||
|| response.getHits().getTotalHits() == hits.length | ||
|| hitRowSet.isLimitReached())) { | ||
// if so, clear the scroll | ||
clear(response.getScrollId(), ActionListener.wrap( | ||
succeeded -> listener.onResponse(new SchemaSearchHitRowSet(schema, exts, hits, query.limit(), null)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reuse the |
||
listener::onFailure)); | ||
} else { | ||
listener.onResponse(new SchemaSearchHitRowSet(schema, exts, hits, query.limit(), scrollId)); | ||
listener.onResponse(hitRowSet); | ||
} | ||
} | ||
// no hits | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,6 @@ | |
class SearchHitRowSet extends AbstractRowSet { | ||
private final SearchHit[] hits; | ||
private final Cursor cursor; | ||
private final String scrollId; | ||
private final List<HitExtractor> extractors; | ||
private final Set<String> innerHits = new LinkedHashSet<>(); | ||
private final String innerHit; | ||
|
@@ -35,7 +34,6 @@ class SearchHitRowSet extends AbstractRowSet { | |
SearchHitRowSet(List<HitExtractor> exts, SearchHit[] hits, int limit, String scrollId) { | ||
|
||
this.hits = hits; | ||
this.scrollId = scrollId; | ||
this.extractors = exts; | ||
|
||
// Since the results might contain nested docs, the iteration is similar to that of Aggregation | ||
|
@@ -91,6 +89,10 @@ class SearchHitRowSet extends AbstractRowSet { | |
} | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Method can have default visibility as it's not used anywhere else. |
||
protected boolean isLimitReached() { | ||
return cursor == Cursor.EMPTY; | ||
} | ||
|
||
@Override | ||
public int columnCount() { | ||
|
@@ -166,10 +168,6 @@ public int size() { | |
return size; | ||
} | ||
|
||
public String scrollId() { | ||
return scrollId; | ||
} | ||
|
||
@Override | ||
public Cursor nextPageCursor() { | ||
return cursor; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick - to remove the
super.fetchSize
redundancy, you can make the method a one liner using the ternary conditional: