-
Notifications
You must be signed in to change notification settings - Fork 25.5k
ESQL: Limit when we push topn to lucene #134497
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
Conversation
Right now we push all topn operations to lucene if possible. But Lucene was not written to handle a topn of 100,000. It's very fast, but it allocates more memory than we'd like. This limits the size of the topns that we push to lucene to the 10,000, which is the default window size limit. We'll run a regular lucene scan with our own in-engine topn instead. That's designed to scan huge numbers of documents. It doesn't have the nice min_competitive optimization. But it tracks memory very well.
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @nik9000, I've created a changelog YAML for you. |
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.
LGTM, thanks Nik!
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.
Only quickly skimmed the change to PushTopNToSource
. Looks alright to me.
if (child instanceof EvalExec evalExec | ||
&& evalExec.child() instanceof EsQueryExec queryExec | ||
&& queryExec.canPushSorts() | ||
&& canPushLimit(topNExec, physicalSettings)) { |
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.
Looks right to me, but I think @craigtaverner should have a look, too. I don't remember if it's bad when we somehow cannot push a WHERE dist < 10 | EVAL ST_DISTANCE = (...)
to Lucene. Craig, we don't end up with un-executable queries, they're just slow when we cannot push, right?
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.
Correct. Failing to push down ST_DISTANCE will not cause the query to fail, just run slow. And as I understand it this PR will only block pushdown for extremely large LIMIT values, which are an extreme case anyway.
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.
10,000
&& queryExec.canPushSorts() | ||
&& canPushDownOrders(topNExec.order(), lucenePushdownPredicates)) { | ||
&& canPushDownOrders(topNExec.order(), lucenePushdownPredicates) | ||
&& canPushLimit(topNExec, physicalSettings)) { |
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.
Looks correct.
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.
LGTM. I also ran some local ST_DISTANCE benchmarks on this branch and saw no changes, which is what I would expect since we don't benchmark very large SORT/LIMIT. But at least it rules out that this change inadvertently breaks ST_DISTANCE pushdown for the cases we've benchmarked.
The thing they broke at first was that they removed the lucene sort with extremely large limits but didn't retain engine sort. I added some more tests to catch that. But, great news on the benchmark! Thanks for looking friends. I'll try and merge this. |
…it_lucene_pushdown
|
||
private static boolean canPushLimit(TopNExec topn, PhysicalSettings physicalSettings) { | ||
return topn.limit() instanceof Literal l && ((Number) l.value()).intValue() <= physicalSettings.luceneTopNLimit(); | ||
} |
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.
NIT: this could be simplified with
Lines 99 to 107 in 2a1176a
public static Integer limitValue(Expression limitField, String sourceText) { | |
if (limitField instanceof Literal literal) { | |
Object value = literal.value(); | |
if (value instanceof Integer intValue) { | |
return intValue; | |
} | |
} | |
throw new EsqlIllegalArgumentException(format(null, "Limit value must be an integer in [{}], found [{}]", sourceText, limitField)); | |
} |
public static final Setting<Integer> LUCENE_TOPN_LIMIT = Setting.intSetting( | ||
"esql.lucene_topn_limit", | ||
IndexSettings.MAX_RESULT_WINDOW_SETTING.getDefault(Settings.EMPTY), | ||
-1, |
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.
We do not limit by default?
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.
Uh, just realized IndexSettings.MAX_RESULT_WINDOW_SETTING.getDefault(Settings.EMPTY)
is a default value
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.
Right! We default to the window's default.
Right now we push all topn operations to lucene if possible. But Lucene was not written to handle a topn of 100,000. It's very fast, but it allocates more memory than we'd like. This limits the size of the topns that we push to lucene to the 10,000, which is the default window size limit. We'll run a regular lucene scan with our own in-engine topn instead. That's designed to scan huge numbers of documents. It doesn't have the nice min_competitive optimization. But it tracks memory very well.
Right now we push all topn operations to lucene if possible. But Lucene was not written to handle a topn of 100,000. It's very fast, but it allocates more memory than we'd like. This limits the size of the topns that we push to lucene to the 10,000, which is the default window size limit. We'll run a regular lucene scan with our own in-engine topn instead. That's designed to scan huge numbers of documents. It doesn't have the nice min_competitive optimization. But it tracks memory very well.
Right now we push all topn operations to lucene if possible. But Lucene was not written to handle a topn of 100,000. It's very fast, but it allocates more memory than we'd like. This limits the size of the topns that we push to lucene to the 10,000, which is the default window size limit. We'll run a regular lucene scan with our own in-engine topn instead. That's designed to scan huge numbers of documents. It doesn't have the nice min_competitive optimization. But it tracks memory very well.