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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Nov 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0f95ef4
[BugFix] Fix incorrect execution plan when generated rewrite in join …
srlch Nov 4, 2024
f924c3f
fix
srlch Nov 4, 2024
d3d513b
fix
srlch Nov 4, 2024
26b327b
add more sql-test
srlch Nov 5, 2024
cccce5f
add sql-test function
srlch Nov 5, 2024
2245617
format
srlch Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions fe/fe-core/src/main/java/com/starrocks/qe/SessionVariable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,8 @@ public static MaterializedViewRewriteMode parse(String str) {

public static final String ENABLE_PLAN_ADVISOR = "enable_plan_advisor";

public static final String DISABLE_GENERATED_COLUMN_REWRITE = "disable_generated_column_rewrite";

public static final List<String> DEPRECATED_VARIABLES = ImmutableList.<String>builder()
.add(CODEGEN_LEVEL)
.add(MAX_EXECUTION_TIME)
Expand Down Expand Up @@ -1587,6 +1589,9 @@ public static MaterializedViewRewriteMode parse(String str) {
@VarAttr(name = ENABLE_COUNT_DISTINCT_REWRITE_BY_HLL_BITMAP)
private boolean enableCountDistinctRewriteByHllBitmap = true;

@VarAttr(name = DISABLE_GENERATED_COLUMN_REWRITE, flag = VariableMgr.INVISIBLE)
private boolean disableGeneratedColumnRewrite = false;

public int getCboPruneJsonSubfieldDepth() {
return cboPruneJsonSubfieldDepth;
}
Expand Down Expand Up @@ -4284,6 +4289,10 @@ public void setEnableCountDistinctRewriteByHllBitmap(boolean enableCountDistinct
this.enableCountDistinctRewriteByHllBitmap = enableCountDistinctRewriteByHllBitmap;
}

public boolean isDisableGeneratedColumnRewrite() {
return disableGeneratedColumnRewrite;
}

public int getConnectorIncrementalScanRangeNumber() {
return connectorIncrementalScanRangeSize;
}
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 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.

Expand Down
3 changes: 3 additions & 0 deletions fe/fe-core/src/main/java/com/starrocks/sql/analyzer/QueryAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ public GeneratedColumnExprMappingCollector() {
}

public Void process(ParseNode node, Scope scope) {
if (session.getSessionVariable().isDisableGeneratedColumnRewrite()) {
return null;
}
return node.accept(this, scope);
}

Expand Down
7 changes: 3 additions & 4 deletions ...e/src/main/java/com/starrocks/sql/optimizer/rewrite/scalar/ReplaceScalarOperatorRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ 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)) {
return m.getValue();
}
if (translateMap.containsKey(scalarOperator)) {
return translateMap.get(scalarOperator);
}

return scalarOperator;
}
}
4 changes: 2 additions & 2 deletions fe/fe-core/src/test/java/com/starrocks/sql/plan/GeneratedColumnTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ public void test() throws Exception {
assertContains(plan, "OUTPUT EXPRS:5: expr");
assertContains(plan, " group by: 1: v1, 2: v2");

sql = " select tmc.v1 + 1 from tmc as v,tmc2 as tmc";
sql = " select v.v1 + 1 from tmc as v,tmc2 as tmc";
plan = getFragmentPlan(sql);
assertContains(plan, "<slot 3> : 3: v3");

sql = " select tmc.v1 + 1 from tmc as v,tmc2 as tmc";
sql = " select v.v1 + 1 from tmc as v,tmc2 as tmc";
plan = getFragmentPlan(sql);
assertContains(plan, "<slot 3> : 3: v3");

Expand Down
10 changes: 10 additions & 0 deletions test/lib/sr_sql_lib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2567,6 +2567,16 @@ def set_first_tablet_bad_and_recover(self, table_name):
else:
break

def assert_is_identical_explain_plan(self, query1, query2):
"""
assert whether two plans from query1 and query2 are identical
"""
sql1 = "explain %s" % query1
sql2 = "explain %s" % query2
res1 = self.execute_sql(sql1, True)
res2 = self.execute_sql(sql2, True)
tools.assert_true(res1 == res2, "assert two plans are different, plan1: {}, plan2: {}".format(res1["result"], res2["result"]))

def assert_explain_contains(self, query, *expects):
"""
assert explain result contains expect string
Expand Down
Loading
Loading