-
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] Support Trino try expression #55144
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Kould <[email protected]>
be/src/exprs/try_expr.cpp
Outdated
return new TryExpr(node); | ||
} | ||
|
||
} |
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 use of LIKELY
macro, which leads to incorrect logic in the evaluate_checked
method.
You can modify the code like this:
StatusOr<ColumnPtr> evaluate_checked(ExprContext* context, Chunk* chunk) override {
auto&& status = _children[0]->evaluate_checked(context, chunk);
if (LIKELY(status.ok())) { // Corrected the condition to check if status is OK
return status;
}
return ColumnHelper::create_const_null_column(chunk->num_rows());
}
return operator; | ||
} | ||
} | ||
|
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:
The equals
method only checks for reference equality, which may not be the intended behavior and could lead to incorrect logical comparison results.
You can modify the code like this:
@Override
public boolean equals(Object other) {
if (this == other) return true;
if (other == null || getClass() != other.getClass()) return false;
TryOperator that = (TryOperator) other;
return Objects.equals(arguments, that.arguments);
}
public <R, C> R accept(AstVisitor<R, C> visitor, C context) { | ||
return visitor.visitTryExpr(this, context); | ||
} | ||
} |
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:
The clone
method calls the TryExpr
constructor with this
, but there's no constructor for TryExpr
that accepts a TryExpr
as an argument, which will result in a compilation error.
You can modify the code like this:
@Override
public Expr clone() {
return new TryExpr(getChild(0).clone(), pos);
}
This change correctly clones the child expression and also provides the current NodePosition
.
f89d30c
to
518857e
Compare
Signed-off-by: Kould <[email protected]>
8dbecf5
to
cc1f41c
Compare
Signed-off-by: Kould <[email protected]>
7645b90
to
0dd2a52
Compare
-- name: test_try_index_out_of_bound | ||
set sql_dialect='trino'; | ||
|
||
select try(array[1, 2, 3][4]); |
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.
please test more cases, like different data types, NULL, exceptions
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 is the only way I can currently test try expressions in starrocks.In trino, when testing cast, an error should occur, but in this case: select cast('s' as int)
, starrocks will return null.
Do you have any test case suggestions?
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.
by default SR doesn't throw exception in expression, but with setting the sql mode like set sql_mode ='ALLOW_THROW_EXCEPTION'
it will throw exception
Signed-off-by: Kould <[email protected]>
a4e75f4
to
db5e4ee
Compare
4e851c2
to
43360dd
Compare
Signed-off-by: Kould <[email protected]>
43360dd
to
15e6b83
Compare
f49f5ce
to
15716a3
Compare
Quality Gate failedFailed conditions |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 0 / 16 (00.00%) file detail
|
Signed-off-by: Kould <[email protected]>
15716a3
to
053afe3
Compare
Why I'm doing:
#54268
What I'm doing:
support try expression on trino SQL dialect
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: