Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KKould
Copy link

@KKould KKould commented Jan 16, 2025

Why I'm doing:

#54268

What I'm doing:

support try expression on trino SQL dialect

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@KKould KKould requested review from a team as code owners January 16, 2025 07:50
@CLAassistant
Copy link

CLAassistant commented Jan 16, 2025

CLA assistant check
All committers have signed the CLA.

@KKould KKould changed the title feat: support Trino try expression [Enhancement] Support Trino try expression Jan 16, 2025
return new TryExpr(node);
}

}
Copy link

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;
}
}

Copy link

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);
}
}
Copy link

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.

@KKould KKould force-pushed the feat/trino_try_expr branch from 8dbecf5 to cc1f41c Compare January 16, 2025 08:04
@github-actions github-actions bot added the 3.4 label Jan 16, 2025
@KKould KKould force-pushed the feat/trino_try_expr branch from 7645b90 to 0dd2a52 Compare January 16, 2025 08:52
-- name: test_try_index_out_of_bound
set sql_dialect='trino';

select try(array[1, 2, 3][4]);
Copy link
Contributor

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

Copy link
Author

@KKould KKould Jan 16, 2025

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?

Copy link
Contributor

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

be/src/exprs/try_expr.cpp Outdated Show resolved Hide resolved
Signed-off-by: Kould <[email protected]>
@KKould KKould force-pushed the feat/trino_try_expr branch from a4e75f4 to db5e4ee Compare January 16, 2025 15:16
@KKould KKould force-pushed the feat/trino_try_expr branch from 4e851c2 to 43360dd Compare January 18, 2025 07:45
@KKould KKould force-pushed the feat/trino_try_expr branch from 43360dd to 15e6b83 Compare January 20, 2025 14:44
@KKould KKould force-pushed the feat/trino_try_expr branch from f49f5ce to 15716a3 Compare January 21, 2025 13:29
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
16.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

Copy link

[BE Incremental Coverage Report]

fail : 0 / 16 (00.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 be/src/exprs/expr.cpp 0 3 00.00% [471, 472, 473]
🔵 be/src/exprs/try_expr.cpp 0 13 00.00% [27, 29, 32, 33, 34, 35, 37, 38, 40, 43, 44, 45, 46]

@KKould KKould force-pushed the feat/trino_try_expr branch from 15716a3 to 053afe3 Compare January 28, 2025 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants