-
Notifications
You must be signed in to change notification settings - Fork 158
[BugFix] Prevent push down limit with offset reach maxResultWindow #3713
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
…ndow Signed-off-by: Heng Qian <[email protected]>
teardown: | ||
- do: | ||
query.settings: | ||
body: | ||
transient: | ||
plugins.calcite.enabled : false | ||
plugins.calcite.fallback.allowed : true |
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.
why we need this teardown step? it seems not enable calcite in setup step.
@penghuo is each xyx.yml independent?
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.
no need revert setting, by default is disabled
query: 'source=test | head 2 | head 1 from 1 ' | ||
- match: { "total": 1 } | ||
- match: { "schema": [ { "name": "id", "type": "bigint" } ] } | ||
- match: { "datarows": [ [ 2 ] ] } |
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.
what's the result without this fixing?
teardown: | ||
- do: | ||
query.settings: | ||
body: | ||
transient: | ||
plugins.calcite.enabled : false | ||
plugins.calcite.fallback.allowed : true |
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, no need revert setting, by default is disabled
teardown: | ||
- do: | ||
query.settings: | ||
body: | ||
transient: | ||
plugins.calcite.enabled : false | ||
plugins.calcite.fallback.allowed : true |
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.
no need revert setting, by default is disabled
Signed-off-by: Heng Qian <[email protected]>
…3713) * [BugFix] Prevent push down limit with offset no less than maxResultWindow Signed-off-by: Heng Qian <[email protected]> * Address comments Signed-off-by: Heng Qian <[email protected]> --------- Signed-off-by: Heng Qian <[email protected]> Signed-off-by: xinyual <[email protected]>
Description
Prevent push down limit with offset reach maxResultWindow
The index setting of
max_result_window
is the maximum restriction offrom + size
.https://docs.opensearch.org/docs/latest/install-and-configure/configuring-opensearch/index-settings/
Thus in our code of building OpenSearch request, we have logic of using
size
equals tomaxResultWindow -statFrom
ifstartFrom + size > maxResultWindow
. It will have problem if we got newsize
equal to 0 or less than 0.We could address this issue by preventing such limit from push down if it would cause the new
startFrom
to reach the maxResultWindow.Related Issues
Resolves #3102
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.