#963 Unimplemented PPL Sort Syntax #994

currantw · 2024-12-16T16:51:31Z

Description

Updates documentation to mark PPL sort syntax (e.g. sort num) as unimplemented.
Temporary measure related to #963, where the consensus seems to be that this syntax should eventually be supported.

Related Issues

OpenSearch SQL issue: opensearch-project/sql#3180

Check List

Updated documentation (docs/ppl-lang/README.md)
~~Implemented unit tests~~
~~Implemented tests for combination with other commands~~
~~New added source code should include a copyright header~~
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.

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

LantaoJin · 2024-12-19T06:47:19Z

I don't think the sort syntax is outdated. But I agree we should remove this syntax since it is unimplemented. How about change the PR title to Remove unimplemented PPL Sort Syntax?

currantw · 2024-12-19T15:36:58Z

I don't think the sort syntax is outdated. But I agree we should remove this syntax since it is unimplemented. How about change the PR title to Remove unimplemented PPL Sort Syntax?

Thanks @LantaoJin. Agree that "unimplemented" is more accurate. Please also see the discussion on the equivalent SQL issue. The consensus there seems to be to keep this syntax for the time being, update all the documentation to indicate that this syntax is deprecated, and remove it completely in the next major release.

As such, I will be updating this PR to merely mark the syntax as deprecated.

Signed-off-by: currantw <[email protected]>

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

YANG-DB

mostly good - just one question about this part:

Compare lengthEqualsZero = new Compare(EQUAL.getName().getFunctionName(), lengthFunction, new Literal(0, DataType.INTEGER));

acarbonetto · 2024-12-19T18:04:09Z

This doesn't resolve #963 - but just deprecates the unimplemented language.
I would link the issues together, but I don't think we should close that issue just because we're deprecating the language.

docs/ppl-lang/ppl-parse-command.md

docs/ppl-lang/PPL-Example-Commands.md

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

Signed-off-by: currantw <[email protected]>

currantw · 2025-01-03T22:47:34Z

Based on the conversation here, it seems like consensus is that we should implement the PPL sort syntax, rather than deprecate and remove it. As such, I have changed this PR to simply mark it as "unimplemented" so that users don't unwittingly use it.

YANG-DB · 2025-01-04T20:05:22Z

...n/src/test/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanParseTranslatorTestSuite.scala

@@ -122,41 +123,47 @@ class PPLLogicalPlanParseTranslatorTestSuite

  test("test parse email & host expressions including cast and sort commands") {


@currantw plz add the corresponding IT (place holder ?) for a concrete index:

val query = "source=t" + " | parse address '(?<streetNumber>\\d+) (?<street>.+)'" + " | eval streetNumberInt = cast(streetNumber as integer)" + " | where streetNumberInt > 500" + " | sort streetNumberInt" + " | fields streetNumber, street"

Signed-off-by: currantw <[email protected]>

YANG-DB · 2025-01-06T18:36:24Z

@currantw LGTM ...

@LantaoJin, @acarbonetto - any additional comments ?

acarbonetto · 2025-01-06T20:36:25Z

docs/ppl-lang/PPL-Example-Commands.md

@@ -274,7 +274,8 @@ source = table |  where ispresent(a) |
 - `source=accounts | parse email '.+@(?<host>.+)' | stats count() by host`
 - `source=accounts | parse email '.+@(?<host>.+)' | eval eval_result=1 | fields host, eval_result`
 - `source=accounts | parse email '.+@(?<host>.+)' | where age > 45 | sort - age | fields age, email, host`
- `source=accounts | parse address '(?<streetNumber>\d+) (?<street>.+)' | where streetNumber > 500 | sort num(streetNumber) | fields streetNumber, street`
+- `source=accounts | parse address '(?<streetNumber>\d+) (?<street>.+)' | eval streetNumberInt = cast(streetNumber as integer) | where streetNumberInt > 500 | sort streetNumberInt | fields streetNumber, street`
+  - **Note**: The `sort num` syntax is not implemented. To sort numerically, cast to a numerical data type - e.g. `sort cast(streetNumber as integer)`. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.


Suggested change

- **Note**: The `sort num` syntax is not implemented. To sort numerically, cast to a numerical data type - e.g. `sort cast(streetNumber as integer)`. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.

- **Note**: The `sort num` syntax is not implemented. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.

acarbonetto · 2025-01-06T20:37:57Z

docs/ppl-lang/ppl-parse-command.md

@@ -68,6 +68,8 @@ PPL query:
    | 880            | Holmes Lane    |
    +----------------+----------------+

+**Note**: The `sort num` syntax is not implemented. To sort numerically, cast to a numerical data type - e.g. `sort cast(streetNumber as integer)`. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.


Someone in the future may get confused by this comment since the sort num syntax is no longer mentioned here.
I'd suggest removing the node completely.

YANG-DB · 2025-01-06T20:44:32Z

@currantw plz check why build is failing

Signed-off-by: currantw <[email protected]>

currantw force-pushed the #963_ppl_sort_syntax branch from b44dad7 to 0e03dd8 Compare December 16, 2024 16:53

YANG-DB reviewed Dec 16, 2024

View reviewed changes

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4 Show resolved Hide resolved

currantw changed the title ~~#963 Remove Outdated PPL Sort Syntax~~ #963 Deprecate Unimplemented PPL Sort Syntax Dec 19, 2024

currantw added 3 commits December 19, 2024 07:43

Initial implementation

46b0f95

Signed-off-by: currantw <[email protected]>

Revert cast changes.

db59830

Signed-off-by: currantw <[email protected]>

Revert changes, and update to mark syntax as deprecated and add TODOs.

a5f35bc

Signed-off-by: currantw <[email protected]>

currantw force-pushed the #963_ppl_sort_syntax branch from 473f0c3 to a5f35bc Compare December 19, 2024 16:34

Revert some unintended changes, minor doc change.

cec3f39

Signed-off-by: currantw <[email protected]>

currantw marked this pull request as ready for review December 19, 2024 16:41

currantw requested review from dai-chen, mengweieric, penghuo, seankao-az, anirudha, kaituo, noCharger, LantaoJin and ykmr1224 as code owners December 19, 2024 16:41

YANG-DB reviewed Dec 19, 2024

View reviewed changes

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java Outdated Show resolved Hide resolved

YANG-DB reviewed Dec 19, 2024

View reviewed changes

acarbonetto reviewed Dec 19, 2024

View reviewed changes

docs/ppl-lang/ppl-parse-command.md Outdated Show resolved Hide resolved

docs/ppl-lang/PPL-Example-Commands.md Outdated Show resolved Hide resolved

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java Outdated Show resolved Hide resolved

currantw added 2 commits December 19, 2024 13:00

Address review comments, including reverting some changes.

953a1fc

Signed-off-by: currantw <[email protected]>

Update sample queries for correctness

9a76e17

Signed-off-by: currantw <[email protected]>

YANG-DB added Lang:PPL Pipe Processing Language support 0.7 labels Dec 19, 2024

currantw added 2 commits December 19, 2024 22:33

Update to use eval for casting

3639399

Signed-off-by: currantw <[email protected]>

Update to mark sort syntax as "unimplemented" rather than "deprecated".

874c63d

Signed-off-by: currantw <[email protected]>

YANG-DB reviewed Jan 4, 2025

View reviewed changes

currantw changed the title ~~#963 Deprecate Unimplemented PPL Sort Syntax~~ #963 Unimplemented PPL Sort Syntax Jan 6, 2025

currantw added 2 commits January 6, 2025 09:30

Update integration test, clarify TODO comments.

0b36545

Signed-off-by: currantw <[email protected]>

Clarify test name, use multi-line string.

e7e8785

Signed-off-by: currantw <[email protected]>

YANG-DB approved these changes Jan 6, 2025

View reviewed changes

acarbonetto approved these changes Jan 6, 2025

View reviewed changes

Fix failing FlintSparkPPLParseITSuite

410b7c0

Signed-off-by: currantw <[email protected]>

YANG-DB approved these changes Jan 7, 2025

View reviewed changes

Remove note completely

6c91f52

Signed-off-by: currantw <[email protected]>

LantaoJin approved these changes Jan 8, 2025

View reviewed changes

acarbonetto approved these changes Jan 9, 2025

View reviewed changes

YANG-DB merged commit 6610253 into opensearch-project:main Jan 9, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#963 Unimplemented PPL Sort Syntax #994

#963 Unimplemented PPL Sort Syntax #994

currantw commented Dec 16, 2024 •

edited

Loading

LantaoJin commented Dec 19, 2024 •

edited

Loading

currantw commented Dec 19, 2024 •

edited

Loading

YANG-DB left a comment

acarbonetto commented Dec 19, 2024

currantw commented Jan 3, 2025

YANG-DB Jan 4, 2025 •

edited

Loading

YANG-DB commented Jan 6, 2025

acarbonetto Jan 6, 2025

acarbonetto Jan 6, 2025

YANG-DB commented Jan 6, 2025

		@@ -122,41 +123,47 @@ class PPLLogicalPlanParseTranslatorTestSuite

		test("test parse email & host expressions including cast and sort commands") {

	- Note: The `sort num` syntax is not implemented. To sort numerically, cast to a numerical data type - e.g. `sort cast(streetNumber as integer)`. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.
	- Note: The `sort num` syntax is not implemented. See [#963](https://github.com/opensearch-project/opensearch-spark/issues/963) for more details.

#963 Unimplemented PPL Sort Syntax #994

#963 Unimplemented PPL Sort Syntax #994

Conversation

currantw commented Dec 16, 2024 • edited Loading

Description

Related Issues

Check List

LantaoJin commented Dec 19, 2024 • edited Loading

currantw commented Dec 19, 2024 • edited Loading

YANG-DB left a comment

Choose a reason for hiding this comment

acarbonetto commented Dec 19, 2024

currantw commented Jan 3, 2025

YANG-DB Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

YANG-DB commented Jan 6, 2025

acarbonetto Jan 6, 2025

Choose a reason for hiding this comment

acarbonetto Jan 6, 2025

Choose a reason for hiding this comment

YANG-DB commented Jan 6, 2025

currantw commented Dec 16, 2024 •

edited

Loading

LantaoJin commented Dec 19, 2024 •

edited

Loading

currantw commented Dec 19, 2024 •

edited

Loading

YANG-DB Jan 4, 2025 •

edited

Loading