[Enhancement] adjust translate function syntax structure to prevent parser performance rollback #54830
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| new FunctionParams(false, visit(context.expression(), Expr.class)), pos); | ||
| return SyntaxSugars.parse(functionCallExpr); | ||
| } | ||
|
|
||
| @Override | ||
| public ParseNode visitAggregationFunctionCall(StarRocksParser.AggregationFunctionCallContext context) { | ||
| NodePosition pos = createPos(context); |
There was a problem hiding this comment.
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);
}
stephen-shelby
force-pushed
the
adjust_translate
branch
2 times, most recently
from
1f639cc to
6b55173
Compare
…arser performance rollback Signed-off-by: stephen <[email protected]>
stephen-shelby
force-pushed
the
adjust_translate
branch
from
6b55173 to
8601bf5
Compare
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
murphyatwork
approved these changes
Jan 8, 2025
Seaven
approved these changes
Jan 9, 2025
kangkaisen
approved these changes
Jan 9, 2025
|
@Mergifyio backport branch-3.4 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jan 9, 2025
…arser performance rollback (#54830) Signed-off-by: stephen <[email protected]> (cherry picked from commit c1bc8d3)
18 tasks
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 9, 2025
…arser performance rollback (backport #54830) (#54856) Co-authored-by: stephen <[email protected]>
|
@Mergifyio backport branch-3.4.0-rc01 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jan 9, 2025
…arser performance rollback (#54830) Signed-off-by: stephen <[email protected]> (cherry picked from commit c1bc8d3) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocksLex.g4 # test/sql/test_agg/R/test_distinct_agg
18 tasks
|
@mergify backport branch-3.3 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jan 31, 2025
…arser performance rollback (#54830) Signed-off-by: stephen <[email protected]> (cherry picked from commit c1bc8d3) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/parser/StarRocks.g4
18 tasks
wanpengfei-git
pushed a commit
that referenced
this pull request
Jan 31, 2025
…arser performance rollback (backport #54830) (#55514) Signed-off-by: yanz <[email protected]> Co-authored-by: stephen <[email protected]> Co-authored-by: yanz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
parser performance rollback is introduced by this patch #54185
parser performance will rollback when there are many function call in query.
the parser of this case will take about 100ms.
bad case:
normal case:
What I'm doing:
Translate statement conflicts with translate function, so stripping translate function from simple function prevents performance rollback.
adjust translate function syntax structure and remove
getCharPositionInLinefortranslateto prevent parser performance rollback.base: 100ms
optimized: 4ms
Fixes #issue
https://github.com/StarRocks/StarRocksTest/issues/8932#issuecomment-2572967332
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: