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

Fix the issue that missing identifiers from ANTLR keywords #821

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Fix the issue that missing identifiers from ANTLR keywords #821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions ppl-spark-integration/src/main/antlr4/OpenSearchPPLLexer.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,9 @@ INDEX: 'INDEX';
D: 'D';
DESC: 'DESC';
DATASOURCES: 'DATASOURCES';
VALUE: 'VALUE';
USING: 'USING';
WITH: 'WITH';

// CLAUSE KEYWORDS
SORTBY: 'SORTBY';
Comment on lines -77 to -82
Copy link
Member Author

Choose a reason for hiding this comment

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

These two keywords are not used at all in parser.


// FIELD KEYWORDS
AUTO: 'AUTO';
STR: 'STR';
Expand Down
106 changes: 54 additions & 52 deletions ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,35 @@ commands
| fieldsummaryCommand
;

commandName
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a new variable commandName, all new command (not function) should be added in this list.

: SEARCH
| DESCRIBE
| SHOW
| AD
| ML
| KMEANS
| WHERE
| CORRELATE
| JOIN
| FIELDS
| STATS
| EVENTSTATS
| DEDUP
| EXPLAIN
| SORT
| HEAD
| TOP
| RARE
| EVAL
| GROK
| PARSE
| PATTERNS
| LOOKUP
| RENAME
| FILLNULL
| FIELDSUMMARY
;

searchCommand
: (SEARCH)? fromClause # searchFrom
| (SEARCH)? fromClause logicalExpression # searchFromFilter
Expand Down Expand Up @@ -360,14 +389,6 @@ statsFunctionName
| STDDEV_POP
;

takeAggFunction
: TAKE LT_PRTHS fieldExpression (COMMA size = integerLiteral)? RT_PRTHS
;

percentileAggFunction
: PERCENTILE LESS value = integerLiteral GREATER LT_PRTHS aggField = fieldExpression RT_PRTHS
;
Comment on lines -363 to -369
Copy link
Member Author

Choose a reason for hiding this comment

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

Never used.

Copy link
Member

Choose a reason for hiding this comment

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

good catch

Copy link
Member

Choose a reason for hiding this comment

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

@kt-eliatra @salyh please make sure why this is not in use

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats not from us and in the g4 files there are plenty of unused statements. And also unused code in the code base btw :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

percentileAgg != percentile (see blame)

OpenSearchPPLParser g4 at 7bc09278d8a9d7df05ff2e086e7f430a4a0da3… 2024-10-28 19-22-32


// expressions
expression
: logicalExpression
Expand Down Expand Up @@ -999,45 +1020,36 @@ keywordsCanBeId
| mathematicalFunctionName
| positionFunctionName
| cryptographicFunctionName
// commands
| SEARCH
| DESCRIBE
| SHOW
| FROM
| WHERE
| CORRELATE
| FIELDS
| RENAME
| STATS
| DEDUP
| SORT
| EVAL
| HEAD
| TOP
| RARE
| PARSE
| METHOD
| REGEX
| PUNCT
| GROK
| PATTERN
| PATTERNS
| NEW_FIELD
| KMEANS
| AD
| ML
| EXPLAIN
| singleFieldRelevanceFunctionName
| multiFieldRelevanceFunctionName
| commandName
| comparisonOperator
| explainMode
| correlationType
// commands assist keywords
| SOURCE
| INDEX
| DESC
| DATASOURCES
// CLAUSEKEYWORDS
| SORTBY
// FIELDKEYWORDSAUTO
| AUTO
| STR
| IP
| NUM
| FROM
| PATTERN
| NEW_FIELD
| SCOPE
| MAPPING
| WITH
| USING
| CAST
| GET_FORMAT
| EXTRACT
| INTERVAL
| PLUS
| MINUS
| INCLUDEFIELDS
| NULLS
// ARGUMENT KEYWORDS
| KEEPEMPTY
| CONSECUTIVE
Expand All @@ -1060,27 +1072,21 @@ keywordsCanBeId
| TRAINING_DATA_SIZE
| ANOMALY_SCORE_THRESHOLD
// AGGREGATIONS
| AVG
| COUNT
| statsFunctionName
| DISTINCT_COUNT
| PERCENTILE
| PERCENTILE_APPROX
| ESTDC
| ESTDC_ERROR
| MAX
| MEAN
| MEDIAN
| MIN
| MODE
| RANGE
| STDEV
| STDEVP
| SUM
| SUMSQ
| VAR_SAMP
| VAR_POP
| STDDEV_SAMP
| STDDEV_POP
| PERCENTILE
| PERCENTILE_APPROX
| TAKE
| FIRST
| LAST
Expand All @@ -1098,10 +1104,6 @@ keywordsCanBeId
| SPARKLINE
| C
| DC
// FIELD SUMMARY
| FIELDSUMMARY
| INCLUDEFIELDS
| NULLS
// JOIN TYPE
| OUTER
| INNER
Expand Down
14 changes: 1 addition & 13 deletions ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.opensearch.sql.ast.expression.DataType;
import org.opensearch.sql.ast.expression.EqualTo;
import org.opensearch.sql.ast.expression.Field;
import org.opensearch.sql.ast.expression.FieldList;
import org.opensearch.sql.ast.expression.Function;
import org.opensearch.sql.ast.expression.subquery.ExistsSubquery;
import org.opensearch.sql.ast.expression.subquery.InSubquery;
Expand All @@ -41,7 +40,6 @@
import org.opensearch.sql.ast.expression.UnresolvedExpression;
import org.opensearch.sql.ast.expression.When;
import org.opensearch.sql.ast.expression.Xor;
import org.opensearch.sql.ast.tree.UnresolvedPlan;
import org.opensearch.sql.common.utils.StringUtils;
import org.opensearch.sql.ppl.utils.ArgumentFactory;

Expand All @@ -53,8 +51,6 @@
import java.util.stream.IntStream;
import java.util.stream.Stream;

import static org.opensearch.flint.spark.ppl.OpenSearchPPLParser.INCLUDEFIELDS;
import static org.opensearch.flint.spark.ppl.OpenSearchPPLParser.NULLS;
import static org.opensearch.sql.expression.function.BuiltinFunctionName.EQUAL;
import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NOT_NULL;
import static org.opensearch.sql.expression.function.BuiltinFunctionName.IS_NULL;
Expand All @@ -80,7 +76,7 @@ public void setAstBuilder(AstBuilder astBuilder) {
/**
* The function name mapping between fronted and core engine.
*/
private static Map<String, String> FUNCTION_NAME_MAPPING =
private static final Map<String, String> FUNCTION_NAME_MAPPING =
new ImmutableMap.Builder<String, String>()
.put("isnull", IS_NULL.getName().getFunctionName())
.put("isnotnull", IS_NOT_NULL.getName().getFunctionName())
Expand Down Expand Up @@ -216,14 +212,6 @@ public UnresolvedExpression visitDistinctCountFunctionCall(OpenSearchPPLParser.D
return new AggregateFunction("count", visit(ctx.valueExpression()), true);
}

@Override
public UnresolvedExpression visitPercentileAggFunction(OpenSearchPPLParser.PercentileAggFunctionContext ctx) {
return new AggregateFunction(
ctx.PERCENTILE().getText(),
visit(ctx.aggField),
Collections.singletonList(new Argument("rank", (Literal) visit(ctx.value))));
}

@Override
public UnresolvedExpression visitPercentileFunctionCall(OpenSearchPPLParser.PercentileFunctionCallContext ctx) {
return new AggregateFunction(
Expand Down
Loading