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

[Feature] support in predict for complex types #26333

Merged
merged 15 commits into from
Jul 11, 2023
Merged

Conversation

fzhedu
Copy link
Contributor

@fzhedu fzhedu commented Jun 30, 2023

  1. array/map/struct support in (value..) and in (subquery), json just support in (value...)
  2. fix map equal bugs
  3. fix stuct type's getCompatibleTypes error
  4. support not equal for array/map/struct
  5. fix cast struct bug of lacking nullable

What type of PR is this:

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

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr will affect users' behaviors
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.1
    • 3.0
    • 2.5
    • 2.4

@mergify mergify bot assigned fzhedu Jun 30, 2023
_const_input.resize(_children.size());
for (auto i = 0; i < _children.size(); ++i) {
if (dynamic_cast<VectorizedLiteral*>(_children[i])) {
ASSIGN_OR_RETURN(_const_input[i], _children[i]->evaluate_checked(context, nullptr));
Copy link
Contributor

@Seaven Seaven Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except for the first one, other children must be constant, I think you can add DCHECK on expr.is_const, and don't need complex handle

Copy link
Contributor Author

@fzhedu fzhedu Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems Expr::is_constant() is not overrided by some expression, and this interface is not really used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as currently rewritting non-const parameters of in predict to OR expressions results in slower performance, we could optimize it by allow in predict to support non-const parameteres, so it is necessary to keep this generial process.

Copy link
Contributor

@Seaven Seaven Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems Expr::is_constant() is not overrided by some expression, and this interface is not really used.

  1. only a few functions doesn't support is_constant, such as random, and we don't support IN on these function, don't worry.
  2. as you say, I think reuse contant expression on IN predicate is unnecessary, but it's only need to modify FE. you can think about it, execut a constant expression once more is faster than execute it as a variable column in your template

Copy link
Contributor

@Seaven Seaven Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, support variable column on IN is not necessarily faster than using OR, it's comparison of row mode and column mode

Seaven
Seaven previously approved these changes Jul 7, 2023
murphyatwork
murphyatwork previously approved these changes Jul 7, 2023
@murphyatwork murphyatwork enabled auto-merge (squash) July 7, 2023 03:53
auto-merge was automatically disabled July 7, 2023 07:16

Head branch was pushed to by a user without write access

@fzhedu fzhedu force-pushed the inPredict branch 2 times, most recently from dbf889b to 3e9491f Compare July 7, 2023 07:17
@fzhedu fzhedu requested a review from a team as a code owner July 7, 2023 07:17
int val_res = _values->equals(i, *(rhs_map._values.get()), j, safe_eq);

// case 1: key_res == EQUALS_TRUE
if (key_res == EQUALS_TRUE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if (key_res == EQUALS_TRUE) { is non-sense.

Copy link
Contributor Author

@fzhedu fzhedu Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not true, it has to diff from the case where key_res == EQUAL_NULL

@fzhedu fzhedu dismissed stale reviews from murphyatwork and Seaven via 1ceab0a July 10, 2023 02:26
fzhedu added 13 commits July 10, 2023 10:26
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
Signed-off-by: Zhuhe Fang <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@wanpengfei-git
Copy link
Collaborator

[FE PR Coverage Check]

😞 fail : 9 / 13 (69.23%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/sql/analyzer/ExpressionAnalyzer.java 3 6 50.00% [800, 801, 807]
🔵 com/starrocks/analysis/InPredicate.java 2 3 66.67% [135]
🔵 com/starrocks/sql/common/TypeManager.java 1 1 100.00% []
🔵 com/starrocks/catalog/Type.java 3 3 100.00% []

@Seaven Seaven merged commit b4a6671 into StarRocks:main Jul 11, 2023
25 checks passed
@fzhedu
Copy link
Contributor Author

fzhedu commented Jul 11, 2023

@mergify backport branch-3.1

@mergify
Copy link
Contributor

mergify bot commented Jul 11, 2023

backport branch-3.1

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jul 11, 2023
1. array/map/struct support in (value..) and in (subquery), json just
support in (value...)
2. fix map equal bugs
3. fix stuct type's getCompatibleTypes error
4. support not equal for array/map/struct
5. fix cast struct bug of lacking nullable

---------

Signed-off-by: Zhuhe Fang <[email protected]>
(cherry picked from commit b4a6671)
wanpengfei-git pushed a commit that referenced this pull request Jul 11, 2023
1. array/map/struct support in (value..) and in (subquery), json just
support in (value...)
2. fix map equal bugs
3. fix stuct type's getCompatibleTypes error
4. support not equal for array/map/struct
5. fix cast struct bug of lacking nullable

---------

Signed-off-by: Zhuhe Fang <[email protected]>
(cherry picked from commit b4a6671)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants