-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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] Add Trino HLL Function Compatibility Mapping and last_day_of_month Support(StarRocks#40894) #47122
Conversation
…ay_of_month Support(StarRocks#40894) Signed-off-by: happut <[email protected]>
…ay_of_month Support(StarRocks#40894) Signed-off-by: happut <[email protected]>
This problem has been resolved with new commit. |
@mergify rerun |
❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚. |
[FE Incremental Coverage Report]✅ pass : 67 / 67 (100.00%) file detail
|
// todo: support more function transform | ||
} | ||
|
||
private static void registerAggregateFunctionTransformer() { | ||
// 1.approx_distinct | ||
registerFunctionTransformer("approx_distinct", 1, | ||
"approx_count_distinct", ImmutableList.of(Expr.class)); |
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.
Why change this? ImmutableList
is fine if use lower java's version(List.of should be used in higher jdk9)?
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.
FunctionCallTransformer transformer = new FunctionCallTransformer(starRocksFunctionCall, trinoFnArgNums); | ||
FunctionCallTransformer transformer; | ||
if (trinoFnArgNums == 0) { | ||
transformer = new FunctionCallTransformer(starRocksFunctionCall, false); |
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.
when trinoFnArgNums == 0, what's the difference of these two ctor. and i think you do not need if-else here
transformer = new FunctionCallTransformer(starRocksFunctionCall, false); | |
transformer = new FunctionCallTransformer(starRocksFunctionCall,trinoFnArgNums); |
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.
I made this change here because the constructor of FunctionCallTransformer handles false and 0 differently.
public FunctionCallTransformer(FunctionCallExpr targetCall, int argNums) {
this(targetCall, false, argNums);
}
public FunctionCallTransformer(FunctionCallExpr targetCall, boolean variableArgument) {
this(targetCall, variableArgument, 0);
}
private FunctionCallTransformer(FunctionCallExpr targetCall, boolean variableArgument, int argNums) {
this.targetCall = targetCall;
this.argNums = argNums;
this.variableArgument = variableArgument;
if (variableArgument) {
this.placeholderExprs = Lists.newArrayList();
} else {
this.placeholderExprs = Arrays.asList(new PlaceholderExpr[argNums]);
}
init();
}
If we do not explicitly set variableArgument to false, the array will be out of bounds.
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.
This ctor already set variableArgument to false. why not just use this ? like FunctionCallTransformer(starRocksFunctionCall, 0);
public FunctionCallTransformer(FunctionCallExpr targetCall, int argNums) {
this(targetCall, false, argNums);
}
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.
Yeah, you are right. 😓
I should be confused ..... here
`if (variableArgument) {`
`
I test this with my computer... wait a moment
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.
Already modified and pushed according to your opinions, the code test is normal, thank you!
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.
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.
I think it should be this ?
this.generatedColumnExpr = column.generatedColumnExpr;
In addition, I feel that there are some problem in this pr.
The file change count looks abnormal....
In fact, the difference between my branch and the main branch is only the following files
Do I need to create a new pr?
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.
sorry , there is compilation problem in main branch, after we fix it, you can rebase it again
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.
#47522 this PR fixed
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.
…ay_of_month Support(StarRocks#40894) Signed-off-by: happut <[email protected]>
…ay_of_month Support(StarRocks#40894) Signed-off-by: happut <[email protected]>
Signed-off-by: before-Sunrise <[email protected]>
Signed-off-by: zhaohehuhu <[email protected]>
Signed-off-by: DanRoscigno <[email protected]> Signed-off-by: 絵空事スピリット <[email protected]> Co-authored-by: 絵空事スピリット <[email protected]>
…rRocks#47013) Signed-off-by: Letian Jiang <[email protected]>
Signed-off-by: Seaven <[email protected]>
…ocks#46892) Signed-off-by: zombee0 <[email protected]>
Signed-off-by: sevev <[email protected]>
Signed-off-by: Seaven <[email protected]>
…ks#47150) Signed-off-by: luohaha <[email protected]>
Signed-off-by: 絵空事スピリット <[email protected]>
Signed-off-by: Kevin Xiaohua Cai <[email protected]>
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Signed-off-by: Letian Jiang <[email protected]>
Signed-off-by: gengjun-git <[email protected]>
Signed-off-by: packy92 <[email protected]>
Signed-off-by: Jiao Mingye <[email protected]>
Quality Gate failedFailed conditions |
Why I'm doing:
In version 3.0 and later, StarRocks supports Trino's SQL_dialect mode; however, ongoing enhancements are necessary to further optimize this functionality, so I added the Trino HLL Function Compatibility Mapping and last_day_of_month Support(#40894)
What I'm doing:
Add Trino HLL Function Compatibility Mapping and last_day_of_month Support(#40894)
Fixes #issue
#40894
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: