Add support for TopN and aggregation pushdown in Elasticsearch#23118
Add support for TopN and aggregation pushdown in Elasticsearch#23118murthy-chelankuri wants to merge 4 commits intotrinodb:masterfrom
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Murthy Chelankuri.
|
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Murthy Chelankuri.
|
f75d92c to
2935da8
Compare
|
Can we split the TopN and aggregation pushdown as a separate commit, this would make the reviews a bit easier |
There was a problem hiding this comment.
It is always a bit recommended to make the object TopN immutable instead of changing its state here
There was a problem hiding this comment.
Thanks @Praveen2112 for the feed back. removed this method and made TopN as immutable.
There was a problem hiding this comment.
Can we revert back the indentations ?
There was a problem hiding this comment.
Thanks @Praveen2112 for the feedback reverted the indentations.
There was a problem hiding this comment.
Can we revert the indentations ?
There was a problem hiding this comment.
I am not see any issue with indentation on my editor. Just compared with the previous version i don't see any change in the indentation. Can you please check once ?
There was a problem hiding this comment.
Instead of maintaining TopN which maintains limit as well - Can we have an Optional<List<SortOrder>> if the list is present we could consider it as TopN else Limit - But we need to handle what if TopN -> Limit and otherway around
There was a problem hiding this comment.
@Praveen2112 , can we keep the topN instead of splitting two arguments Optional<List> and limit? But if that is what we need to do, we can look into changing accordingly.
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
c85c5eb to
02c2107
Compare
|
This pull request has gone a while without any activity. Tagging for triage help: @mosabua |
|
Added stale-ignore labels since I think @murthy-chelankuri is wanting to continue work on this .. do you know what your next steps are @murthy-chelankuri or do you need help from reviewers? |
|
You need to rebase this PR @murthy-chelankuri to proceed .. it is currently not reviewable |
13032b5 to
02c2107
Compare
|
Thank you @murthy-chelankuri for building this! Any more updates on this? |
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
Description
This pull request adds support for pushing down TopN and Aggregation to Elasticsearch.
Additional context and related issues
Opening a new merge request because the previous one was automatically closed for being stale, and we couldn't reopen the stale pull request.
#16919
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: