-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Enhancement] Update regexp_extract function for trino parser #34845
[Enhancement] Update regexp_extract function for trino parser #34845
Conversation
|
// regexp_extract -> if(regexp_extract(xxx)='', null, regexp_extract(xxx)) | ||
registerFunctionTransformer("regexp_extract", 2, | ||
new FunctionCallExpr("if", ImmutableList.of(predicate, new NullLiteral(), regexpExtractFunc)) | ||
); | ||
} | ||
|
||
private static void registerJsonFunctionTransformer() { |
There was a problem hiding this comment.
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:
Incorrect argument replacement in the placeholder transformation for regexp_extract
function call.
You can modify the code like this:
@@ -222,13 @@ private static void registerRegexpFunctionTransformer() {
// regexp_extract(string, pattern) -> regexp_extract(str, pattern, 0)
FunctionCallExpr regexpExtractFunc = new FunctionCallExpr("regexp_extract",
ImmutableList.of(new PlaceholderExpr(0, Expr.class), new PlaceholderExpr(1, Expr.class), new IntLiteral(0L)));
- BinaryPredicate predicate = new BinaryPredicate(BinaryType.EQ, regexpExtractFunc, new StringLiteral(""));
+ BinaryPredicate predicate = new BinaryPredicate(BinaryType.EQ, new PlaceholderExpr(0, Expr.class), new StringLiteral(""));
// regexp_extract -> if(regexp_extract(xxx)='', null, regexp_extract(xxx))
registerFunctionTransformer("regexp_extract", 2,
new FunctionCallExpr("if", ImmutableList.of(predicate, new NullLiteral(), regexpExtractFunc))
);
}
In the above modification, by changing regexpExtractFunc
to new PlaceholderExpr(0, Expr.class)
in the BinaryPredicate
, we ensure that the first argument passed (xxx
) is correctly compared with an empty string and then used in both places of the conditional expression within the if
statement. Otherwise, regexpExtractFunc
would be evaluated twice which is not necessary or intended.
Note: As this snippet does not contain full context or complete code, I am assuming that the indexing of placeholders follows zero-based indexing which is typical in Java (and many other programming languages). If, however, the original code indeed uses one-based indexing for placeholders (which would be atypical), the original placeholder indices should be preserved, and my correction would not be accurate.
8fa6e83
to
ff163a7
Compare
Signed-off-by: zenoyang <cookie.yz@qq.com>
ff163a7
to
0eeb1dc
Compare
f0a5b6e
to
6449423
Compare
Kudos, SonarCloud Quality Gate passed!
|
@Mergifyio backport branch-3.2 |
@Mergifyio backport branch-3.1 |
✅ Backports have been created
|
@Mergifyio backport branch-3.0 |
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: zenoyang <cookie.yz@qq.com> (cherry picked from commit c9cd165)
Signed-off-by: zenoyang <cookie.yz@qq.com> (cherry picked from commit c9cd165)
Signed-off-by: zenoyang <cookie.yz@qq.com> (cherry picked from commit c9cd165)
[FE Incremental Coverage Report]✅ pass : 5 / 5 (100.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 2 / 2 (100.00%) file detail
|
Signed-off-by: zenoyang <cookie.yz@qq.com> (cherry picked from commit c9cd165)
Signed-off-by: zenoyang <cookie.yz@qq.com> (cherry picked from commit c9cd165)
Why I'm doing:
In StarRocks, the regexp_extract function returns an empty string when there is no matching content. Trino returns NULL. The two behave inconsistently.
What I'm doing:
Function conversion in trino compatibility layer:
regexp_extract -> if(regexp_extract(xxx)= ", null, regexp_extract(xxx))
Fixes #34026
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: