[Enhancement] Use sql_mode to be compatible with legacy group_concat #36150
Conversation
Signed-off-by: liuyehcf <[email protected]>
![starrocks-cr[bot]](https://avatars.githubusercontent.com/in/404334?s=60&v=4){kind=small}
| String sep = ","; | ||
| sepExpr = new StringLiteral(sep, pos); | ||
| exprs.add(sepExpr); | ||
| } | ||
| } | ||
| if (!orderByElements.isEmpty()) { | ||
| int exprSize = exprs.size(); |
There was a problem hiding this comment.
The most risky bug in this code is:
Logic for checking isLegacyGroupConcat and adding the separator expression may cause unwanted behavior in cases where exprs.size() does not equal 1.
You can modify the code like this:
if (isLegacyGroupConcat) {
// The original logic assumes there should be only a single expression
// for legacy group concat, but does not handle other cases.
// We need to define what should happen if exprs.size() is not 1.
Expr sepExpr;
String sep = exprs.size() == 1 ? ", " : /* some default or error handling */;
sepExpr = new StringLiteral(sep, pos);
exprs.add(sepExpr);
} else {
Expr sepExpr;
String sep = ",";
sepExpr = new StringLiteral(sep, pos);
exprs.add(sepExpr);
}Assuming that the case where exprs.size() is not 1 is either an error condition or requires a default fallback, the modification ensures safe handling of all possible sizes of exprs. Please note that it's important to decide on the expected behavior when exprs.size() does not equal 1 - my suggestion is to handle this with either an error or a default value, but this depends on specific application logic.
Signed-off-by: liuyehcf <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 1 / 1 (100.00%) file detail
|
|
@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
|
…36150) Signed-off-by: liuyehcf <[email protected]> (cherry picked from commit 11ece99)
…36150) Signed-off-by: liuyehcf <[email protected]> (cherry picked from commit 11ece99)
…36150) Signed-off-by: liuyehcf <[email protected]> (cherry picked from commit 11ece99) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java
…(backport #36150) (#36318) Co-authored-by: liuyehcf <[email protected]>
…36150) Signed-off-by: liuyehcf <[email protected]> (cherry picked from commit 11ece99)
…(backport #36150) (#36317) Co-authored-by: liuyehcf <[email protected]>
…(backport #36150) (#36319) Signed-off-by: liuyehcf <[email protected]> Co-authored-by: liuyehcf <[email protected]>
…tarRocks#36150) Signed-off-by: liuyehcf <[email protected]> Signed-off-by: 鼎昕 <[email protected]>
|
version >= 3.0.9 or >=3.1.6 or >=3.2.1 |
Why I'm doing:
After #28778, the signature of group_concat has been changed. So for users that already used this feature, they need to update the sql as well which may include a lot of work.
What I'm doing:
Add a sql mode
GROUP_CONCAT_LEGACYto support the syntax the way the were. make it work well with the legacy sql base.Limitation
This work around process happens at parser stage. so var hint like
/*+ SET_VAR(sql_mode = 'GROUP_CONCAT_LEGACY')*/won't workWhat 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: