Skip to content

Fix asc/desc keyword behavior for sort command #4651

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
Merged
RyanL1997 merged 9 commits into from
Oct 28, 2025
Merged

Fix asc/desc keyword behavior for sort command #4651

RyanL1997 merged 9 commits into from
Oct 28, 2025

Conversation

@ritvibhatt
Copy link
Contributor

@ritvibhatt ritvibhatt commented Oct 23, 2025 •
edited
Loading

Description

Fixed PPL sort command so that asc/desc keywords specify sort direction for individual fields instead of applying all fields in the sort command. (keywords work on individual fields ex sort field1 asc, field2 desc means field1 is ascending and field2 is descending) while preventing mixing of prefix (+/-) and suffix (asc/desc) syntax, and updated integration tests to match this behavior.

Files Changed

  • ppl/src/main/antlr/OpenSearchPPLParser.g4: Updated grammar rules for sort command to support individual field ASC/DESC keywords
  • ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java: Updated visitSortCommand() method to handle new ASC/DESC field syntax and prevent mixing of +/-and asc/desc sort syntax
  • ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java: Modified expression building logic for sort field parsing and ensure consistent sort direction syntax usage across all sort fields
  • ppl/src/main/java/org/opensearch/sql/ppl/utils/ArgumentFactory.java: Updated getSortArguments() method to process new field-level ASC/DESC syntax

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

@ritvibhatt
update asc/desc behavior for sort
e4f55cf 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
fix both sort directions on same field error
3619cc3 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
fix formatting
eb6536e 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
fix tests
2c8f5ea 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
fix tests
aeb9fea 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
update failing tests
647cc90 
Signed-off-by: Ritvi Bhatt <[email protected]>
@LantaoJin LantaoJin added the enhancement New feature or request label Oct 24, 2025
@ritvibhatt ritvibhatt changed the title Update asc/desc keyword behavior for sort command Fix asc/desc keyword behavior for sort command Oct 24, 2025
@ritvibhatt ritvibhatt marked this pull request as ready for review October 24, 2025 15:52
ykmr1224
ykmr1224 reviewed Oct 24, 2025
View reviewed changes
ppl/src/main/antlr/OpenSearchPPLParser.g4

sortField
: (PLUS | MINUS)? sortFieldExpression
: (PLUS | MINUS) sortFieldExpression (ASC | A | DESC | D) # invalidMixedSortField
Copy link
Collaborator

@ykmr1224 ykmr1224 Oct 24, 2025

Choose a reason for hiding this comment

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

Why do we need to define invalid syntax? wouldn't it fail parse without this?

Copy link
Contributor Author

@ritvibhatt ritvibhatt Oct 24, 2025

Choose a reason for hiding this comment

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

It would fail the parse but added this to have a more clear error for when the two syntaxes are mixed

ykmr1224
ykmr1224 reviewed Oct 24, 2025
View reviewed changes
RyanL1997
RyanL1997 reviewed Oct 28, 2025
View reviewed changes
Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @ritvibhatt , thanks for the change, and I left some minor comments.

RyanL1997
RyanL1997 previously approved these changes Oct 28, 2025
View reviewed changes
@ritvibhatt
fix formatting
00ddb41 
Signed-off-by: Ritvi Bhatt <[email protected]>
@ritvibhatt
fix formatting
18e09f0 
Signed-off-by: Ritvi Bhatt <[email protected]>
@RyanL1997 RyanL1997 merged commit 448c42a into opensearch-project:main Oct 28, 2025
53 of 54 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 28, 2025
@github-actions
Fix asc/desc keyword behavior for sort command (#4651)
d67c120 
(cherry picked from commit 448c42a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 28, 2025
Merged
ykmr1224 pushed a commit that referenced this pull request Oct 29, 2025
@opensearch-trigger-bot @github-actions
Fix asc/desc keyword behavior for sort command (#4651) (#4691)
cea4893 
(cherry picked from commit 448c42a)

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
expani pushed a commit to vinaykpud/sql that referenced this pull request Nov 4, 2025
@ritvibhatt @expani
Fix asc/desc keyword behavior for sort command (opensearch-project#4651)
5872b1a
sandeshkr419 added a commit to sandeshkr419/sql that referenced this pull request Dec 3, 2025
@sandeshkr419 @github-actions
@Swiddis @manasvinibs @opensearch-ci-bot @noCharger @dai-chen @Mebsina @yuancu @opensearch-trigger-bot @ahkcs @penghuo @alexey-temnikov @RileyJergerAmazon @ykmr1224 @LantaoJin @songkant-aws @qianheng-aws @xinyual @RyanL1997 @peterzhuamazon @vinaykpud @expani @vamsimanohar @ritvibhatt @ishaoxy @mch2
Data fusion changes
6f7fd68 
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Simeon Widdis <[email protected]>
Co-authored-by: Manasvini B S <[email protected]>
Co-authored-by: opensearch-ci-bot <[email protected]>
Co-authored-by: Louis Chu <[email protected]>
Co-authored-by: Chen Dai <[email protected]>
Co-authored-by: Mebsina <[email protected]>
Co-authored-by: Yuanchun Shen <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Kai Huang <[email protected]>
Co-authored-by: Peng Huo <[email protected]>
Co-authored-by: Alexey Temnikov <[email protected]>
Co-authored-by: Riley Jerger <[email protected]>
Co-authored-by: Tomoyuki MORITA <[email protected]>
Co-authored-by: Lantao Jin <[email protected]>
Co-authored-by: Songkan Tang <[email protected]>
Co-authored-by: qianheng <[email protected]>
Co-authored-by: Simeon Widdis <[email protected]>
Co-authored-by: Xinyuan Lu <[email protected]>
Co-authored-by: Jialiang Liang <[email protected]>
Co-authored-by: Peter Zhu <[email protected]>
Co-authored-by: Vinay Krishna Pudyodu <[email protected]>
Co-authored-by: expani <[email protected]>
Co-authored-by: expani1729 <[email protected]>
Co-authored-by: Vamsi Manohar <[email protected]>
Co-authored-by: ritvibhatt <[email protected]>
Co-authored-by: Xinyu Hao <[email protected]>
Co-authored-by: Marc Handalian <[email protected]>
Co-authored-by: Marc Handalian <[email protected]>
Fix join type ambiguous issue when specify the join type with sql-like join criteria (opensearch-project#4474)
Fix issue 4441 (opensearch-project#4449)
Fix missing keywordsCanBeId (opensearch-project#4491)
Fix the bug of explicit makeNullLiteral for UDT fields (opensearch-project#4475)
Fix mapping after aggregation push down (opensearch-project#4500)
Fix percentile bug (opensearch-project#4539)
Fix JsonExtractAllFunctionIT failure (opensearch-project#4556)
Fix sort push down into agg after project already pushed (opensearch-project#4546)
Fix push down failure for min/max on derived field (opensearch-project#4572)
Fix compile issue in main (opensearch-project#4608)
Fix filter parsing failure on date fields with non-default format (opensearch-project#4616)
Fix bin nested fields issue (opensearch-project#4606)
Fix: Support Alias Fields in MIN, MAX, FIRST, LAST, and TAKE Aggregations (opensearch-project#4621)
fix rename issue (opensearch-project#4670)
Fixes for `Multisearch` and `Append` command (opensearch-project#4512)
Fix asc/desc keyword behavior for sort command (opensearch-project#4651)
Fix] Fix unexpected shift of extraction for `rex` with nested capture groups in named groups  (opensearch-project#4641)
Fix CVE-2025-48924 (opensearch-project#4665)
Fix sub-fields accessing of generated structs (opensearch-project#4683)
Fix] Incorrect Field Index Mapping in AVG to SUM/COUNT Conversion (opensearch-project#15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ykmr1224 ykmr1224 ykmr1224 approved these changes

@RyanL1997 RyanL1997 RyanL1997 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

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

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

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

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

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

@penghuo penghuo Awaiting requested review from penghuo penghuo 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

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

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

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

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

Assignees

No one assigned

Labels

backport 2.19-dev enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ritvibhatt @ykmr1224 @RyanL1997 @LantaoJin