starrocks icon indicating copy to clipboard operation
starrocks copied to clipboard

[BugFix] fix an issue that user-defined variables sql unable to handle variable dependencies

Open zhangheihei opened this issue 1 year ago • 11 comments

Why I'm doing:

What I'm doing:

Fixes #47370

What type of PR is this:

  • [x] BugFix
  • [ ] Feature
  • [ ] Enhancement
  • [ ] Refactor
  • [ ] UT
  • [ ] Doc
  • [ ] Tool

Does this PR entail a change in behavior?

  • [x] 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
  • [x] Policy changes: use new policy to replace old one, functionality automatically enabled
  • [ ] Feature removed
  • [ ] Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • [x] 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:

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

zhangheihei avatar Jun 22 '24 17:06 zhangheihei

@zhangheihei can you add the case mentioned in the issue in the sql-test cases to cover the fix.

imay avatar Jun 23 '24 00:06 imay

@zhangheihei can you add the case mentioned in the issue in the sql-test cases to cover the fix.

hi @imay the ut added

zhangheihei avatar Jun 23 '24 15:06 zhangheihei

@Mergifyio rebase

zhangheihei avatar Jun 24 '24 02:06 zhangheihei

rebase

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • [ ] any of:
    • [ ] sender-permission>=read
    • [ ] sender=github-actions[bot]

mergify[bot] avatar Jun 24 '24 02:06 mergify[bot]

@Mergifyio rebase main

trueeyu avatar Jun 24 '24 02:06 trueeyu

rebase main

✅ Nothing to do for rebase action

mergify[bot] avatar Jun 24 '24 02:06 mergify[bot]

I see that UserVariable is an AST. Can it be calculated iteratively during analysis? Why do we need to reAnalyze for different UserVariables? I think every UserVariable be analyzed once.

HangyuanLiu avatar Jun 24 '24 03:06 HangyuanLiu

I see that UserVariable is an AST. Can it be calculated iteratively during analysis? Why do we need to reAnalyze for different UserVariables? I think every UserVariable be analyzed once.

Scenario 1: During analysis, variables that UserVariable depends on may not be written to ConnectContext.userVariables Scenario 2: During analysis, UserVariable depends on variables that may be old values. @HangyuanLiu

zhangheihei avatar Jun 24 '24 13:06 zhangheihei

[FE Incremental Coverage Report]

:white_check_mark: pass : 67 / 69 (97.10%)

file detail

path covered_line new_line coverage not_covered_line_detail
:large_blue_circle: com/starrocks/qe/StmtExecutor.java 0 1 00.00% [811]
:large_blue_circle: com/starrocks/sql/analyzer/SetStmtAnalyzer.java 30 31 96.77% [102]
:large_blue_circle: com/starrocks/sql/ast/UserVariable.java 4 4 100.00% []
:large_blue_circle: com/starrocks/qe/ConnectContext.java 4 4 100.00% []
:large_blue_circle: com/starrocks/qe/SetExecutor.java 13 13 100.00% []
:large_blue_circle: com/starrocks/sql/analyzer/ExpressionAnalyzer.java 16 16 100.00% []

github-actions[bot] avatar Jul 13 '24 10:07 github-actions[bot]

[BE Incremental Coverage Report]

:white_check_mark: pass : 0 / 0 (0%)

github-actions[bot] avatar Jul 13 '24 10:07 github-actions[bot]