-
Notifications
You must be signed in to change notification settings - Fork 180
Support reverse command with Calcite
#3867
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
Changes from all commits
03dd144
4d0ea18
1ed7db0
50b2155
087c936
ca43a8c
92b0aa6
6a35a5d
71fdb1a
998f311
e03f3e2
22b859f
6362ee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.ast.tree; | ||
|
|
||
| import com.google.common.collect.ImmutableList; | ||
| import java.util.List; | ||
| import lombok.EqualsAndHashCode; | ||
| import lombok.Getter; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.Setter; | ||
| import lombok.ToString; | ||
| import org.opensearch.sql.ast.AbstractNodeVisitor; | ||
|
|
||
| /** AST node represent Reverse operation. */ | ||
| @Getter | ||
| @Setter | ||
| @ToString | ||
| @EqualsAndHashCode(callSuper = false) | ||
| @RequiredArgsConstructor | ||
| public class Reverse extends UnresolvedPlan { | ||
|
|
||
| private UnresolvedPlan child; | ||
|
|
||
| @Override | ||
| public Reverse attach(UnresolvedPlan child) { | ||
| this.child = child; | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public List<UnresolvedPlan> getChild() { | ||
| return this.child == null ? ImmutableList.of() : ImmutableList.of(this.child); | ||
| } | ||
|
|
||
| @Override | ||
| public <T, C> T accept(AbstractNodeVisitor<T, C> nodeVisitor, C context) { | ||
| return nodeVisitor.visitReverse(this, context); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| ============= | ||
| reverse | ||
| ============= | ||
|
|
||
| .. rubric:: Table of contents | ||
|
|
||
| .. contents:: | ||
| :local: | ||
| :depth: 2 | ||
|
|
||
|
|
||
| Description | ||
| ============ | ||
| | Using ``reverse`` command to reverse the display order of search results. The same results are returned, but in reverse order. | ||
|
|
||
| Version | ||
| ======= | ||
| 3.2.0 | ||
|
|
||
| Syntax | ||
| ============ | ||
| reverse | ||
|
|
||
selsong marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| * No parameters: The reverse command takes no arguments or options. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a note to call out limitation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks!
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| Note | ||
| ===== | ||
| The `reverse` command processes the entire dataset. If applied directly to millions of records, it will consume significant memory resources on the coordinating node. Users should only apply the `reverse` command to smaller datasets, typically after aggregation operations. | ||
|
|
||
| Example 1: Basic reverse operation | ||
| ================================== | ||
|
|
||
| The example shows reversing the order of all documents. | ||
|
|
||
| PPL query:: | ||
|
|
||
| os> source=accounts | fields account_number, age | reverse; | ||
| fetched rows / total rows = 4/4 | ||
| +----------------+-----+ | ||
| | account_number | age | | ||
| |----------------+-----| | ||
| | 6 | 36 | | ||
| | 18 | 33 | | ||
| | 1 | 32 | | ||
| | 13 | 28 | | ||
| +----------------+-----+ | ||
|
|
||
|
|
||
| Example 2: Reverse with sort | ||
| ============================ | ||
|
|
||
| The example shows reversing results after sorting by age in ascending order, effectively giving descending order. | ||
|
|
||
| PPL query:: | ||
|
|
||
| os> source=accounts | sort age | fields account_number, age | reverse; | ||
| fetched rows / total rows = 4/4 | ||
| +----------------+-----+ | ||
| | account_number | age | | ||
| |----------------+-----| | ||
| | 6 | 36 | | ||
| | 18 | 33 | | ||
| | 1 | 32 | | ||
| | 13 | 28 | | ||
| +----------------+-----+ | ||
|
|
||
|
|
||
| Example 3: Reverse with head | ||
| ============================ | ||
|
|
||
| The example shows using reverse with head to get the last 2 records from the original order. | ||
|
|
||
| PPL query:: | ||
|
|
||
| os> source=accounts | reverse | head 2 | fields account_number, age; | ||
| fetched rows / total rows = 2/2 | ||
| +----------------+-----+ | ||
| | account_number | age | | ||
| |----------------+-----| | ||
| | 6 | 36 | | ||
| | 18 | 33 | | ||
| +----------------+-----+ | ||
|
|
||
|
|
||
| Example 4: Double reverse | ||
| ========================= | ||
|
|
||
| The example shows that applying reverse twice returns to the original order. | ||
|
|
||
| PPL query:: | ||
|
|
||
| os> source=accounts | reverse | reverse | fields account_number, age; | ||
| fetched rows / total rows = 4/4 | ||
| +----------------+-----+ | ||
| | account_number | age | | ||
| |----------------+-----| | ||
| | 13 | 28 | | ||
| | 1 | 32 | | ||
| | 18 | 33 | | ||
| | 6 | 36 | | ||
| +----------------+-----+ | ||
|
|
||
|
|
||
| Example 5: Reverse with complex pipeline | ||
| ======================================= | ||
|
|
||
| The example shows reverse working with filtering and field selection. | ||
|
|
||
| PPL query:: | ||
|
|
||
| os> source=accounts | where age > 30 | fields account_number, age | reverse; | ||
| fetched rows / total rows = 3/3 | ||
| +----------------+-----+ | ||
| | account_number | age | | ||
| |----------------+-----| | ||
| | 6 | 36 | | ||
| | 18 | 33 | | ||
| | 1 | 32 | | ||
| +----------------+-----+ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.sql.calcite.remote; | ||
|
|
||
| import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; | ||
| import static org.opensearch.sql.util.MatcherUtils.rows; | ||
| import static org.opensearch.sql.util.MatcherUtils.schema; | ||
| import static org.opensearch.sql.util.MatcherUtils.verifyDataRowsInOrder; | ||
| import static org.opensearch.sql.util.MatcherUtils.verifySchema; | ||
|
|
||
| import java.io.IOException; | ||
| import org.json.JSONObject; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.opensearch.sql.ppl.PPLIntegTestCase; | ||
|
|
||
| public class CalciteReverseCommandIT extends PPLIntegTestCase { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could u add a test in ExplainIT for reverse command? |
||
|
|
||
| @Override | ||
| public void init() throws Exception { | ||
| super.init(); | ||
| enableCalcite(); | ||
| disallowCalciteFallback(); | ||
| loadIndex(Index.BANK); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverse() throws IOException { | ||
| JSONObject result = | ||
| executeQuery(String.format("source=%s | fields account_number | reverse", TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder( | ||
| result, rows(32), rows(25), rows(20), rows(18), rows(13), rows(6), rows(1)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverseWithFields() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "source=%s | fields account_number, firstname | reverse", TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint"), schema("firstname", "string")); | ||
| verifyDataRowsInOrder( | ||
| result, | ||
| rows(32, "Dillard"), | ||
| rows(25, "Virginia"), | ||
| rows(20, "Elinor"), | ||
| rows(18, "Dale"), | ||
| rows(13, "Nanette"), | ||
| rows(6, "Hattie"), | ||
| rows(1, "Amber JOHnny")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverseWithSort() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "source=%s | sort account_number | fields account_number | reverse", | ||
| TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder( | ||
| result, rows(32), rows(25), rows(20), rows(18), rows(13), rows(6), rows(1)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDoubleReverse() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "source=%s | fields account_number | reverse | reverse", TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder( | ||
| result, rows(1), rows(6), rows(13), rows(18), rows(20), rows(25), rows(32)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverseWithHead() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format("source=%s | fields account_number | reverse | head 3", TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder(result, rows(32), rows(25), rows(20)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverseWithComplexPipeline() throws IOException { | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "source=%s | where account_number > 18 | fields account_number | reverse | head 2", | ||
| TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder(result, rows(32), rows(25)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReverseWithMultipleSorts() throws IOException { | ||
| // Use the existing BANK data but with a simpler, more predictable query | ||
| JSONObject result = | ||
| executeQuery( | ||
| String.format( | ||
| "source=%s | sort account_number | fields account_number | reverse | head 3", | ||
| TEST_INDEX_BANK)); | ||
| verifySchema(result, schema("account_number", "bigint")); | ||
| verifyDataRowsInOrder(result, rows(32), rows(25), rows(20)); | ||
| } | ||
| } | ||
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.
IMO, reverse by row_number should only apply to the plan (RelNode) which doesn't contain collation. For example:
source=t | ... | sort - age | reverseshould apply sort by age asc, rather than row_number.You can try:
Add
reverseCollation()to PlanUtils.classUh oh!
There was an error while loading. Please reload this page.
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.
Then add some tests:
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.
@LantaoJin @selsong is it correct issue? or performance imporvement?
If it is performance improvement, we can track by issue #3924. And raise 2nd PR.
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.
Thanks for the suggestion! This is a performance improvement, and it's already tracked under issue #3924. The current implementation still produces correct results, but applying reverseCollation where applicable would avoid unnecessary computation. I'll address this enhancement in a follow-up PR.
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.
I don't think my suggestion is a pushdown enhancement, but is okey to refactor in separate PR.