-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Data] Add updated essql
expression function
#132332
Conversation
Pinging @elastic/kibana-app-services (Team:AppServicesSv) |
|
||
return lastValueFrom( | ||
search.search<EssqlSearchStrategyRequest, EssqlSearchStrategyResponse>(req, { | ||
strategy: ESSQL_SEARCH_STRATEGY, |
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.
So canvas now uses a new essql function that uses a new generic strategy from data plugin?
Compared to the new generic strategy, canvas ESSQL_SEARCH_STRATEGY did internal pagination through scroll till the end server-side.
If I understand correctly, then canvas now lost it now, correct?
If yes, then we need to add it back somewhere.
Options I was thinking of:
- Add it as an option to search strategy and do it internally server-side as it was before (bad because doesn't fit into async search and search polling idea until we support client-side search strategies)
- Handle it on the expression function level, maybe fit it into the partial results framework
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 did the scroll achieve that we don't with the new strategy ? canvas on the client received the list of all results. It will get the same now ?
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.
canvas has the same behavior, seems they intended to support larger requests at some point but feature newer worked.
we will switch to the new strategy and we can think about supporting larger requests in the future.
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.
canvas essql search strategy can be deleted as well
@ppisljar we cannot delete that yet, it is used in the canvas's |
This seems to have the same issue as the canvas essql function - it doesn't respect the
This gives me 1000 rows. But
will give me only 100 rows in Canvas |
bd2bda1
to
c863d0b
Compare
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
@flash1293 that's a strange bug with count. It looks like it only happens when you are doing the render as debug though. If you just do straight render, you'll get a table paginated through the expected number of results. |
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.
Presentation changes look good to me. New function works as expected in Canvas.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Resolves #129478.
Checklist
For maintainers