[BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name #52584

srlch · 2024-11-04T09:51:51Z

Why I'm doing:

This problem is introduced by pr #50398. In this pr, we introduce some new rule for generated column rewriting. The basic idea is following:

Collect the rewriting relation in every SELECT scope in query. (Expr -> SlotRef)
Translate the expression relation into Operator mapping: ScalarOperator -> ColumnRefOperator
Introduce new rule says ReplaceScalarOperatorRule, use this new rule to replace the ScalarOperator by ColumnRefOperator when generating the logical plan in optimizer.

This problem is that, ReplaceScalarOperatorRule use ScalarOperator.isEquivalent to check if a ScalarOperator hit the rule instead of using ScalarOperator.equals. ScalarOperator.isEquivalent does not check the operator id but this id will be used to identify the column with the same column name but come from different table in JOIN relation. (e.g column xx in TABLE A and column xx in TABLE B has same name but different id, in this case, ScalarOperator.isEquivalent return true but ScalarOperator.equals return false). So in this case, we will get the wrong mapping and generated a incorrect plan for generated column rewrite.

What I'm doing:

Using ScalarOperator.equals instead
Introduce session variables disable_generated_column_rewrite for disable the generated column rewrite if we want.

Fixes #issue

What type of PR is this:

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:

…relation if left table and right table has the same column name Why I'm doing: This problem is introduced by pr StarRocks#50398. In this pr, we introduce some new rule for generated column rewriting. The basic idea is following: 1. Collect the rewriting relation in every SELECT scope in query. (Expr -> ColumnRef) 2. Translate the expression relation into Operator mapping: ScalarOperator -> ColumnRefOperator 3. Introduce new rule says ReplaceScalarOperatorRule, use this new rule to replace the ScalarOperator into ColumnRefOperator when generating the logical plan in optimizer. This problem is that, ReplaceScalarOperatorRule use ScalarOperator.isEquivalent to check if a ScalarOperator is hit the rule instead of using ScalarOperator.equals. ScalarOperator.isEquivalent does not check the operator id but this id will be used to indentify the column with the same column name but come from different table in JOIN relation. (e.g column xx in TABLE A and column xx in TABLE B has same name but different id, in this case, ScalarOperator.isEquivalent return true but ScalarOperator.equals return false). So in this case, we will get the wrong mapping and generated a incorrent plan for generated column rewrite. What I'm doing: 1. Using ScalarOperator.equals instead 2. Introduce session variables disable_generated_column_rewrite for disable the generated column rewrite if we want. Signed-off-by: srlch <[email protected]>

starrocks-cr · 2024-11-04T09:52:31Z

fe/fe-core/src/main/java/com/starrocks/qe/SessionVariable.java

+    public void setDisableGeneratedColumnRewrite(boolean disableGeneratedColumnRewrite) {
+        this.disableGeneratedColumnRewrite = disableGeneratedColumnRewrite;
+    }
+
    public int getConnectorIncrementalScanRangeNumber() {
        return connectorIncrementalScanRangeSize;
    }


The most risky bug in this code is:
The DISABLE_GENERATED_COLUMN_REWRITE variable may not be accounted for in the method that applies rewrites. If other flags interact with rewrite behavior, this oversight could cause unexpected issues.

You can modify the code like this:

// Assuming there is a method where rewrite decisions are applied. // We need to ensure DISABLE_GENERATED_COLUMN_REWRITE is checked and used to control logic, // particularly in any code path that performs or skips rewrites. public void applyRewriteRules() { if (isDisableGeneratedColumnRewrite()) { // Add logic to skip generated column rewrite here } else { // Existing rewrite logic } }

Ensure that wherever rewrite rules are applied, you include logic to handle the disableGeneratedColumnRewrite flag.

murphyatwork · 2024-11-04T10:26:16Z

...core/src/main/java/com/starrocks/sql/optimizer/rewrite/scalar/ReplaceScalarOperatorRule.java

@@ -30,7 +30,7 @@ public ReplaceScalarOperatorRule(Map<ScalarOperator, ColumnRefOperator> translat
    @Override
    public ScalarOperator visit(ScalarOperator scalarOperator, ScalarOperatorRewriteContext context) {
        for (Map.Entry<ScalarOperator, ColumnRefOperator> m : translateMap.entrySet()) {
-            if (ScalarOperator.isEquivalent(m.getKey(), scalarOperator)) {
+            if (m.getKey().equals(scalarOperator)) {


why not use Map.contains

Signed-off-by: srlch <[email protected]>

sonarcloud · 2024-11-05T06:45:20Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-11-05T08:30:16Z

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-11-05T08:31:10Z

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-11-05T08:31:11Z

[FE Incremental Coverage Report]

✅ pass : 6 / 6 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/sql/optimizer/rewrite/scalar/ReplaceScalarOperatorRule.java	2	2	100.00%	[]
🔵	com/starrocks/qe/SessionVariable.java	2	2	100.00%	[]
🔵	com/starrocks/sql/analyzer/QueryAnalyzer.java	2	2	100.00%	[]

github-actions · 2024-11-05T10:21:39Z

@Mergifyio backport branch-3.3

github-actions · 2024-11-05T10:21:41Z

@Mergifyio backport branch-3.2

mergify · 2024-11-05T10:21:43Z

backport branch-3.3

✅ Backports have been created

#52638 [BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name (backport #52584) has been created for branch branch-3.3 but encountered conflicts

mergify · 2024-11-05T10:21:48Z

backport branch-3.2

✅ Backports have been created

#52639 [BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name (backport #52584) has been created for branch branch-3.2 but encountered conflicts

…n join relation if left table and right table has the same column name (#52584) Signed-off-by: srlch <[email protected]> (cherry picked from commit af20339) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/qe/SessionVariable.java

…n join relation if left table and right table has the same column name (StarRocks#52584) Signed-off-by: srlch <[email protected]>

…n join relation if left table and right table has the same column name (backport #52584) (#52643) Signed-off-by: srlch <[email protected]>

…n join relation if left table and right table has the same column name (backport #52584) (#52644) Signed-off-by: srlch <[email protected]>

…n join relation if left table and right table has the same column name (StarRocks#52584) Signed-off-by: srlch <[email protected]> Signed-off-by: zhiminr.ren <[email protected]>

srlch requested review from a team as code owners November 4, 2024 09:51

mergify bot assigned srlch Nov 4, 2024

starrocks-cr bot reviewed Nov 4, 2024

View reviewed changes

github-actions bot added behavior_changed 3.3 3.2 labels Nov 4, 2024

murphyatwork reviewed Nov 4, 2024

View reviewed changes

fix

f924c3f

Signed-off-by: srlch <[email protected]>

murphyatwork previously approved these changes Nov 4, 2024

View reviewed changes

Seaven previously approved these changes Nov 4, 2024

View reviewed changes

fix

d3d513b

Signed-off-by: srlch <[email protected]>

srlch dismissed stale reviews from Seaven and murphyatwork via d3d513b November 4, 2024 14:21

srlch added 3 commits November 5, 2024 14:34

add more sql-test

26b327b

Signed-off-by: srlch <[email protected]>

add sql-test function

cccce5f

Signed-off-by: srlch <[email protected]>

format

2245617

Signed-off-by: srlch <[email protected]>

murphyatwork approved these changes Nov 5, 2024

View reviewed changes

Seaven approved these changes Nov 5, 2024

View reviewed changes

murphyatwork approved these changes Nov 5, 2024

View reviewed changes

murphyatwork merged commit af20339 into StarRocks:main Nov 5, 2024
69 checks passed

github-actions bot removed the 3.3 label Nov 5, 2024

github-actions bot removed the 3.2 label Nov 5, 2024

Seaven pushed a commit that referenced this pull request Nov 6, 2024

[BugFix] Fix incorrect execution plan when generated column rewrite i…

1385ff5

…n join relation if left table and right table has the same column name (backport #52584) (#52643) Signed-off-by: srlch <[email protected]>

github-actions bot added the 3.2-merged label Nov 6, 2024

Seaven pushed a commit that referenced this pull request Nov 6, 2024

[BugFix] Fix incorrect execution plan when generated column rewrite i…

327d65b

…n join relation if left table and right table has the same column name (backport #52584) (#52644) Signed-off-by: srlch <[email protected]>

github-actions bot added the 3.3-merged label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name #52584

[BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name #52584

srlch commented Nov 4, 2024 •

edited

Loading

starrocks-cr bot Nov 4, 2024

murphyatwork Nov 4, 2024

srlch Nov 4, 2024

sonarcloud bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

mergify bot commented Nov 5, 2024 •

edited

Loading

mergify bot commented Nov 5, 2024 •

edited

Loading

[BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name #52584

[BugFix] Fix incorrect execution plan when generated column rewrite in join relation if left table and right table has the same column name #52584

Conversation

srlch commented Nov 4, 2024 • edited Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

starrocks-cr bot Nov 4, 2024

Choose a reason for hiding this comment

murphyatwork Nov 4, 2024

Choose a reason for hiding this comment

srlch Nov 4, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Nov 5, 2024

Quality Gate passed

github-actions bot commented Nov 5, 2024

[Java-Extensions Incremental Coverage Report]

github-actions bot commented Nov 5, 2024

[BE Incremental Coverage Report]

github-actions bot commented Nov 5, 2024

[FE Incremental Coverage Report]

file detail

github-actions bot commented Nov 5, 2024

github-actions bot commented Nov 5, 2024

mergify bot commented Nov 5, 2024 • edited Loading

✅ Backports have been created

mergify bot commented Nov 5, 2024 • edited Loading

✅ Backports have been created

srlch commented Nov 4, 2024 •

edited

Loading

mergify bot commented Nov 5, 2024 •

edited

Loading

mergify bot commented Nov 5, 2024 •

edited

Loading