Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add TakeOrderedOperator #2863

Merged
merged 11 commits into from
Aug 5, 2024
Merged

add TakeOrderedOperator #2863

merged 11 commits into from
Aug 5, 2024

Conversation

qianheng-aws
Copy link
Contributor

@qianheng-aws qianheng-aws commented Jul 26, 2024 •
edited by LantaoJin
Loading

Description

add TakeOrderedOperator

Issues Resolved

#2857

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --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.

@qianheng-aws
add TakeOrderedOperator
c940a0d 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
add visitTakeOrdered
b03cfc0 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
fix doc description and increase code coverage to 1.0
9ba4be0 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
address comments and fix doctest
8b2b5e5 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
spotlessApply
c24d2db 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
make SortHelper coverage 1.0
4b25b72 
Signed-off-by: Heng Qian <qianheng@amazon.com>
penghuo
penghuo previously approved these changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the change!
@dai-chen please take a look.

core/src/main/java/org/opensearch/sql/planner/physical/SortHelper.java Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/planner/physical/TakeOrderedOperator.java
this.sortList = sortList;
this.limit = limit;
this.offset = offset;
this.ordering = SortHelper.constructExprOrdering(sortList);
Copy link
Collaborator

@penghuo penghuo Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job on simpification!

@qianheng-aws
Add comments for SortHelper
cf2bc7a 
Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
Merge remote-tracking branch 'refs/remotes/origin/main' into takeOrde…
c3450ea 
…redOperator

# Conflicts:
#	docs/user/optimization/optimization.rst
@qianheng-aws
spotlessApply
a984dc1 
Signed-off-by: Heng Qian <qianheng@amazon.com>
LantaoJin
LantaoJin reviewed Aug 2, 2024
View reviewed changes
core/src/test/java/org/opensearch/sql/planner/physical/TakeOrderedOperatorTest.java Outdated
assertThat(
execute(
takeOrdered(
inputPlan, 2, 1, Pair.of(SortOption.DEFAULT_ASC, ref("response", INTEGER)))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test which its limit equals its offset?

Copy link
Contributor Author

@qianheng-aws qianheng-aws Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it comes to me that we may not need to do that test since Limit(offset, limit) in OpenSearch has different semantics with other engine. For OpenSearch, limit stands for the size of HEAD and offset stands for the from of HEAD.
So it will always return limit rows no matter which value offset is.
e.g.

POST _plugins/_ppl
{
  "query": """
    source=opensearch_dashboards_sample_data_flights| fields FlightTimeMin | head 2 from 2
  """
}

# response
{
  "schema": [
    {
      "name": "FlightTimeMin",
      "type": "float"
    }
  ],
  "datarows": [
    [
      0
    ],
    [
      222.74905
    ]
  ],
  "total": 2,
  "size": 2
}

Copy link
Member

@LantaoJin LantaoJin Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it will always return limit rows no matter which value offset is.

Will skip(offset) in open() method of TakeOrderedOperator change the current behavior?
Nice to have a test to compare the results between take-ordered and limit+head.

Copy link
Contributor Author

@qianheng-aws qianheng-aws Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it's not exact right to say it will always return limit rows no matter which value offset is actually. It's right only when the size of input rows is no less than offset + limit.

I think skip(offset) is semantic equal to what LimitOperator do in its open(). You can see the expected results for each UT, I've add different limit, offset cases for each sort options. But I'll add one more test to verify that's also what sort + limit will produce.

@qianheng-aws
Add more tests to verify the takeOrdered is semantic equal to sort + …
a1e27f6 
…limit

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws
refine test code
8c2aa19 
Signed-off-by: Heng Qian <qianheng@amazon.com>
dai-chen
dai-chen approved these changes Aug 2, 2024
View reviewed changes
Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

In future, I think it'd great to have micro-benchmark in benchmarks module to demonstrate such improvements.

LantaoJin
LantaoJin approved these changes Aug 3, 2024
View reviewed changes
Copy link
Member

@LantaoJin LantaoJin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@penghuo penghuo merged commit 0e70a50 into opensearch-project:main Aug 5, 2024
14 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 5, 2024
@github-actions
add TakeOrderedOperator (#2863)
867a15e 
---------
Signed-off-by: Heng Qian <qianheng@amazon.com>
(cherry picked from commit 0e70a50)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Aug 5, 2024
Closed
LantaoJin pushed a commit that referenced this pull request Aug 7, 2024
@qianheng-aws @github-actions
[Backport 2.x] add TakeOrderedOperator (#2863)(#2906)
6efb805 
* add TakeOrderedOperator (#2863)

---------
Signed-off-by: Heng Qian <qianheng@amazon.com>
(cherry picked from commit 0e70a50)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Make it compatible with java11

Signed-off-by: Heng Qian <qianheng@amazon.com>

---------

Signed-off-by: Heng Qian <qianheng@amazon.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@LantaoJin LantaoJin mentioned this pull request Aug 7, 2024
Merged
manasvinibs pushed a commit to manasvinibs/sql that referenced this pull request Aug 14, 2024
@qianheng-aws @manasvinibs
add TakeOrderedOperator (opensearch-project#2863)
39a6712 
---------
Signed-off-by: Heng Qian <qianheng@amazon.com>
jzonthemtn pushed a commit to jzonthemtn/sql that referenced this pull request Aug 28, 2024
@qianheng-aws @jzonthemtn
add TakeOrderedOperator (opensearch-project#2863)
39b6ebb 
---------
Signed-off-by: Heng Qian <qianheng@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@penghuo penghuo penghuo left review comments

@LantaoJin LantaoJin LantaoJin approved these changes

@dai-chen dai-chen dai-chen approved these changes

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@rupal-bq rupal-bq Awaiting requested review from rupal-bq rupal-bq is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsi-amazon vamsi-amazon Awaiting requested review from vamsi-amazon vamsi-amazon is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

Assignees
No one assigned
Labels
backport 2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qianheng-aws @LantaoJin @penghuo @dai-chen