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.

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

P1 Antlr grammar #859

Merged
merged 10 commits into from
Nov 2, 2024

Conversation

YANG-DB
Copy link
Member

@YANG-DB YANG-DB commented Oct 31, 2024

Description

Add latest p1 commands into the Antlr grammar:

  • flatten
  • geoip
  • trendline
  • expand

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.

Comment on lines +257 to +258
: FLATTEN fieldExpression
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: is the indent level correct? (save for below)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is the same with the following
kmeansCommand, adCommand and so on...

Copy link
Member

Choose a reason for hiding this comment

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

There are no right or wrong about 3 spaces or 4 spaces as indent level. All indent level should be same.

;

expandCommand
: EXPAND fieldExpression
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we use expand instead of expand_field? (ref)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

* The flatten command implemented

Signed-off-by: Lukasz Soszynski <[email protected]>

* The flatten command integration tests were extended with additional checks for logical plans.

Signed-off-by: Lukasz Soszynski <[email protected]>

* flatten, added more tests related to plan translation and integration tests

Signed-off-by: Lukasz Soszynski <[email protected]>

* Flatten command added to command names list.

Signed-off-by: Lukasz Soszynski <[email protected]>

---------

Signed-off-by: Lukasz Soszynski <[email protected]>
@YANG-DB YANG-DB requested a review from ykmr1224 October 31, 2024 22:39
@@ -396,6 +403,9 @@ ISPRESENT: 'ISPRESENT';
BETWEEN: 'BETWEEN';
CIDRMATCH: 'CIDRMATCH';

// Geo Loction
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: Loction -> Location

* add sourceTables to MV index metadata properties

Signed-off-by: Sean Kao <[email protected]>

* parse source tables from mv query

Signed-off-by: Sean Kao <[email protected]>

* test cases for parse source tables from mv query

Signed-off-by: Sean Kao <[email protected]>

* use constant for metadata cache version

Signed-off-by: Sean Kao <[email protected]>

* write source tables to metadata cache

Signed-off-by: Sean Kao <[email protected]>

* address comment

Signed-off-by: Sean Kao <[email protected]>

* generate source tables for old mv without new prop

Signed-off-by: Sean Kao <[email protected]>

* syntax fix

Signed-off-by: Sean Kao <[email protected]>

---------

Signed-off-by: Sean Kao <[email protected]>
| FILLNULL
| FIELDSUMMARY
| TRENDLINE
| GEOIP
Copy link
Member

Choose a reason for hiding this comment

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

GETOIP is not a command name

Comment on lines +257 to +258
: FLATTEN fieldExpression
;
Copy link
Member

Choose a reason for hiding this comment

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

There are no right or wrong about 3 spaces or 4 spaces as indent level. All indent level should be same.

@@ -516,6 +544,11 @@ dataTypeFunctionCall
: CAST LT_PRTHS expression AS convertedDataType RT_PRTHS
;

// geoip function
geoipFunctionCall
: GEOIP LT_PRTHS (datasource = functionArg COMMA)? ipAddress = functionArg (COMMA properties = stringLiteral)? RT_PRTHS
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking why not reusing evalFunctionCall? I don't see any special syntax definition here. And IP function sounds not one of the primary expressions.

noCharger and others added 4 commits November 1, 2024 06:23
…-project#850)

* Fallback to internal scheduler when index creation failed

Signed-off-by: Louis Chu <[email protected]>

* Fix IT

Signed-off-by: Louis Chu <[email protected]>

* Fix IOException

Signed-off-by: Louis Chu <[email protected]>

---------

Signed-off-by: Louis Chu <[email protected]>
* WIP trendline command

Signed-off-by: Kacper Trochimiak <[email protected]>

* wip

Signed-off-by: Kacper Trochimiak <[email protected]>

* trendline supports sorting

Signed-off-by: Kacper Trochimiak <[email protected]>

* run scalafmtAll

Signed-off-by: Kacper Trochimiak <[email protected]>

* return null when there are too few data points

Signed-off-by: Kacper Trochimiak <[email protected]>

* sbt scalafmtAll

Signed-off-by: Kacper Trochimiak <[email protected]>

* Remove WMA references

Signed-off-by: Hendrik Saly <[email protected]>

* trendline - sortByField as Optional<Field>

Signed-off-by: Kacper Trochimiak <[email protected]>

* introduce TrendlineStrategy

Signed-off-by: Kacper Trochimiak <[email protected]>

* keywordsCanBeId -> replace SMA with trendlineType

Signed-off-by: Kacper Trochimiak <[email protected]>

* handle trendline alias as qualifiedName instead of fieldExpression

Signed-off-by: Kacper Trochimiak <[email protected]>

* Add docs

Signed-off-by: Hendrik Saly <[email protected]>

* Make alias optional

Signed-off-by: Hendrik Saly <[email protected]>

* Adapt tests for optional alias

Signed-off-by: Hendrik Saly <[email protected]>

* Adden logical plan unittests

Signed-off-by: Hendrik Saly <[email protected]>

* Add missing license headers

Signed-off-by: Hendrik Saly <[email protected]>

* Fix docs

Signed-off-by: Hendrik Saly <[email protected]>

* numberOfDataPoints must be 1 or greater

Signed-off-by: Hendrik Saly <[email protected]>

* Rename TrendlineStrategy to  TrendlineCatalystUtils

Signed-off-by: Hendrik Saly <[email protected]>

* Validate TrendlineType early and pass around enum type

Signed-off-by: Hendrik Saly <[email protected]>

* Add trendline chaining test

Signed-off-by: Hendrik Saly <[email protected]>

* Fix compile errors

Signed-off-by: Hendrik Saly <[email protected]>

* Fix imports

Signed-off-by: Hendrik Saly <[email protected]>

* Fix imports

Signed-off-by: Hendrik Saly <[email protected]>

---------

Signed-off-by: Kacper Trochimiak <[email protected]>
Signed-off-by: Hendrik Saly <[email protected]>
Co-authored-by: Kacper Trochimiak <[email protected]>
# Conflicts:
#	ppl-spark-integration/src/main/antlr4/OpenSearchPPLLexer.g4
#	ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4
@YANG-DB YANG-DB merged commit 0e733f5 into opensearch-project:p1-antlr-grammar Nov 2, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants