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

[Enhancement] adjust translate function syntax structure to prevent parser performance rollback #54830

Merged
merged 1 commit into from
Jan 9, 2025
+33 −13
Merged

[Enhancement] adjust translate function syntax structure to prevent parser performance rollback #54830

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
12 changes: 11 additions & 1 deletion fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6857,7 +6857,6 @@ private static void addArgumentUseTypeInt(Expr value, List<Expr> exprs) {

@Override
public ParseNode visitSimpleFunctionCall(StarRocksParser.SimpleFunctionCallContext context) {

String fullFunctionName = getQualifiedName(context.qualifiedName()).toString();
NodePosition pos = createPos(context);

Expand Down Expand Up @@ -7057,6 +7056,17 @@ public ParseNode visitSimpleFunctionCall(StarRocksParser.SimpleFunctionCallConte
return SyntaxSugars.parse(functionCallExpr);
}

@Override
public ParseNode visitTranslateFunctionCall(StarRocksParser.TranslateFunctionCallContext context) {
String fullFunctionName = context.TRANSLATE().getText();
NodePosition pos = createPos(context);

FunctionName fnName = FunctionName.createFnName(fullFunctionName);
FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName,
new FunctionParams(false, visit(context.expression(), Expr.class)), pos);
return SyntaxSugars.parse(functionCallExpr);
}

@Override
public ParseNode visitAggregationFunctionCall(StarRocksParser.AggregationFunctionCallContext context) {
NodePosition pos = createPos(context);
Copy link

Choose a reason for hiding this comment

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

The most risky bug in this code is:
Potential incorrect handling of the expression() method which could lead to runtime exceptions if it doesn't return a list that matches expected parameters for FunctionParams.

You can modify the code like this:

@Override
public ParseNode visitTranslateFunctionCall(StarRocksParser.TranslateFunctionCallContext context) {
    String fullFunctionName = "translate";
    NodePosition pos = createPos(context);

    List<Expr> expressionList = visit(context.expression(), Expr.class);
    if (expressionList == null || expressionList.size() < expectedSize) {
        // Handle error or exception appropriately
        throw new IllegalArgumentException("Invalid number of arguments for translate function.");
    }

    FunctionName fnName = FunctionName.createFnName(fullFunctionName);
    FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName,
            new FunctionParams(false, expressionList), pos);
    return SyntaxSugars.parse(functionCallExpr);
}

Expand Down
3 changes: 2 additions & 1 deletion fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2478,6 +2478,7 @@ functionCall
| specialFunctionExpression #specialFunction
| aggregationFunction over? #aggregationFunctionCall
| windowFunction over #windowFunctionCall
| TRANSLATE '(' (expression (',' expression)*)? ')' #translateFunctionCall
| qualifiedName '(' (expression (',' expression)*)? ')' over? #simpleFunctionCall
;

Expand Down Expand Up @@ -2974,7 +2975,7 @@ nonReserved
| SAMPLE | SCHEDULE | SCHEDULER | SECOND | SECURITY | SEPARATOR | SERIALIZABLE |SEMI | SESSION | SETS | SIGNED | SNAPSHOT | SNAPSHOTS | SQLBLACKLIST | START
| STREAM | SUM | STATUS | STOP | SKIP_HEADER | SWAP
| STORAGE| STRING | STRUCT | STATS | SUBMIT | SUSPEND | SYNC | SYSTEM_TIME
| TABLES | TABLET | TABLETS | TAG | TASK | TEMPORARY | TIMESTAMP | TIMESTAMPADD | TIMESTAMPDIFF | THAN | TIME | TIMES | TRANSACTION | TRACE
| TABLES | TABLET | TABLETS | TAG | TASK | TEMPORARY | TIMESTAMP | TIMESTAMPADD | TIMESTAMPDIFF | THAN | TIME | TIMES | TRANSACTION | TRACE | TRANSLATE
| TRIM_SPACE
| TRIGGERS | TRUNCATE | TYPE | TYPES
| UNBOUNDED | UNCOMMITTED | UNSET | UNINSTALL | USAGE | USER | USERS | UNLOCK
Expand Down
2 changes: 1 addition & 1 deletion fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocksLex.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ TIMESTAMPADD: 'TIMESTAMPADD';
TIMESTAMPDIFF: 'TIMESTAMPDIFF';
TINYINT: 'TINYINT';
TRANSACTION: 'TRANSACTION';
TRANSLATE: {getCharPositionInLine() == 0}? 'TRANSLATE';
TRANSLATE: 'TRANSLATE';
TO: 'TO';
TRACE: 'TRACE';
TRIGGERS: 'TRIGGERS';
Expand Down
12 changes: 12 additions & 0 deletions fe/fe-core/src/test/java/com/starrocks/sql/parser/ParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.antlr.v4.runtime.CommonTokenStream;
import org.antlr.v4.runtime.atn.PredictionMode;
import org.junit.Assert;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -578,6 +579,17 @@ private static Stream<Arguments> unexpectedTokenSqls() {
return arguments.stream();
}

@Test
public void testTranslateFunction() {
String sql = "select translate('abcabc', 'ab', '12') as test;";
SessionVariable sessionVariable = new SessionVariable();
try {
SqlParser.parse(sql, sessionVariable);
} catch (Exception e) {
Assertions.fail("sql should success. errMsg: " + e.getMessage());
}
}

}


11 changes: 4 additions & 7 deletions test/sql/test_agg/R/test_distinct_agg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,9 @@ select count(distinct t1.c1, if(t2.c3 is null, 1, 0)), sum(t1.c2 + if(t2.c3 is n
-- result:
1 5000
-- !result
# streaming_preaggregation_mode = 'auto' new_planner_agg_stage = '0'
-- streaming_preaggregation_mode = 'auto' new_planner_agg_stage = '0'
set streaming_preaggregation_mode = 'auto';
-- result:
E: (1064, "Getting syntax error at line 1, column 2. Detail message: Unexpected input 'streaming_preaggregation_mode', the most similar input is {'ADMIN', 'GRANT', TRANSLATE, 'STOP', 'TRUNCATE', '(', ';'}.")
-- !result
set new_planner_agg_stage = '0';
-- result:
Expand Down Expand Up @@ -489,10 +488,9 @@ select array_agg(distinct c2), count(distinct c2), sum(c3) from skew_agg group b
[3] 1 5000
[3] 1 5000
-- !result
# streaming_preaggregation_mode = 'force_streaming' new_planner_agg_stage = '2'
-- streaming_preaggregation_mode = 'force_streaming' new_planner_agg_stage = '2'
set streaming_preaggregation_mode = 'force_streaming';
-- result:
E: (1064, "Getting syntax error at line 1, column 2. Detail message: Unexpected input 'streaming_preaggregation_mode', the most similar input is {'ADMIN', 'GRANT', TRANSLATE, 'STOP', 'TRUNCATE', '(', ';'}.")
-- !result
set new_planner_agg_stage = '0';
-- result:
Expand Down Expand Up @@ -737,10 +735,9 @@ select array_agg(distinct c2), count(distinct c2), sum(c3) from skew_agg group b
[3] 1 5000
[3] 1 5000
-- !result
# streaming_preaggregation_mode = 'force_preaggregation' new_planner_agg_stage = '2'
-- streaming_preaggregation_mode = 'force_preaggregation' new_planner_agg_stage = '2'
set streaming_preaggregation_mode = 'force_preaggregation';
-- result:
E: (1064, "Getting syntax error at line 1, column 2. Detail message: Unexpected input 'streaming_preaggregation_mode', the most similar input is {'ADMIN', 'GRANT', TRANSLATE, 'STOP', 'TRUNCATE', '(', ';'}.")
-- !result
set new_planner_agg_stage = '0';
-- result:
Expand Down Expand Up @@ -1008,4 +1005,4 @@ select sum(c0), count(distinct c2, c3) from skew_agg where c0 = 480816 and c1 =
select count(distinct 1, 2), count(distinct 1), count(distinct null), group_concat(distinct 'a', 'b'), count(distinct c2) from skew_agg;
-- result:
1 1 0 ab 1
-- !result
-- !result
6 changes: 3 additions & 3 deletions test/sql/test_agg/T/test_distinct_agg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ select count(distinct c1, c2), count(distinct c2, c3), sum(c2) from skew_agg lim
select count(distinct c1, c2, c3), count(distinct c2, c3), sum(c2) from skew_agg group by c4 limit 500;
select count(distinct t1.c1, if(t2.c3 is null, 1, 0)), sum(t1.c2 + if(t2.c3 is null, 1, 0)) from skew_agg t1 left join skew_agg t2 on t1.c4 > t2.c4 group by t1.c4 limit 500;

# streaming_preaggregation_mode = 'auto' new_planner_agg_stage = '0'
-- streaming_preaggregation_mode = 'auto' new_planner_agg_stage = '0'
set streaming_preaggregation_mode = 'auto';
set new_planner_agg_stage = '0';
select group_concat(distinct c2, upper(c4) order by abs(c2 + c3)), array_agg(c3 order by 1, c4), ceil(sum(c5)) from (select * from skew_agg order by 1,2,3,4,5,6 limit 10) t;
Expand Down Expand Up @@ -171,7 +171,7 @@ select group_concat(distinct c2), array_agg(distinct c2), count(distinct c2), su
select array_agg(distinct c2), count(distinct c2), sum(c3) from skew_agg group by rollup(c1, c2);


# streaming_preaggregation_mode = 'force_streaming' new_planner_agg_stage = '2'
-- streaming_preaggregation_mode = 'force_streaming' new_planner_agg_stage = '2'
set streaming_preaggregation_mode = 'force_streaming';
set new_planner_agg_stage = '0';
select group_concat(distinct c2, upper(c4) order by abs(c2 + c3)), array_agg(c3 order by 1, c4), ceil(sum(c5)) from (select * from skew_agg order by 1,2,3,4,5,6 limit 10) t;
Expand Down Expand Up @@ -247,7 +247,7 @@ select group_concat(distinct c2), array_agg(distinct c2), count(distinct c2), su
select array_agg(distinct c2), count(distinct c2), sum(c3) from skew_agg group by rollup(c1, c2);


# streaming_preaggregation_mode = 'force_preaggregation' new_planner_agg_stage = '2'
-- streaming_preaggregation_mode = 'force_preaggregation' new_planner_agg_stage = '2'
set streaming_preaggregation_mode = 'force_preaggregation';
set new_planner_agg_stage = '0';
select group_concat(distinct c2, upper(c4) order by abs(c2 + c3)), array_agg(c3 order by 1, c4), ceil(sum(c5)) from (select * from skew_agg order by 1,2,3,4,5,6 limit 10) t;
Expand Down
Loading