plan: use the inferred type as the column type in the schema #7624
Conversation
zz-jason
added
status/WIP
type/bugfix
plan/logical_plan_builder.go
Outdated
| @@ -594,6 +596,9 @@ func (b *planBuilder) buildDistinct(child LogicalPlan, length int) *LogicalAggre | |||
| } | |||
| plan4Agg.SetChildren(child) | |||
| plan4Agg.SetSchema(child.Schema().Clone()) | |||
| for i, col := range plan4Agg.schema.Columns { | |||
| col.RetType = plan4Agg.AggFuncs[i].RetTp | |||
| } | |||
| // Distinct will be rewritten as first_row, we reset the type here since the return type | |||
There was a problem hiding this comment.
this function already has this type update in next few lines.
plan/rule_decorrelate.go
Outdated
| first := aggregation.NewAggFuncDesc(agg.ctx, ast.AggFuncFirstRow, []expression.Expression{col}, false) | ||
| newAggFuncs = append(newAggFuncs, first) | ||
| outerPlan.Schema().Columns[i].RetType = first.RetTp |
There was a problem hiding this comment.
Is it safe to modify the child schema directly? should we make a clone here?
| @@ -115,7 +115,9 @@ func (b *planBuilder) buildAggregation(p LogicalPlan, aggFuncList []*ast.Aggrega | |||
| for _, col := range p.Schema().Columns { | |||
| newFunc := aggregation.NewAggFuncDesc(b.ctx, ast.AggFuncFirstRow, []expression.Expression{col}, false) | |||
| plan4Agg.AggFuncs = append(plan4Agg.AggFuncs, newFunc) | |||
| schema4Agg.Append(col) | |||
| newCol, _ := col.Clone().(*expression.Column) | |||
There was a problem hiding this comment.
So one column occurs in agg and agg' child. The tp of this two is different?
There was a problem hiding this comment.
Possibly. Actually, they are two columns if the type inferred after NewAggFuncDesc() is different from the origin type.
There was a problem hiding this comment.
Since they are different, why unique id is the same one.
|
/run-all-tests |
|
@winoros PTAL |
|
@zz-jason Should we let the columns have same |
|
@winoros I think the |
|
/run-all-tests |
|
@zz-jason Should we keep the |
|
/run-common-test |
|
/run-common-test |
|
I've figured out why new column cannot take effect easily. Will try to fix it when i have time. |
zz-jason
added
status/LGT2
What problem does this PR solve?
fix #7578
fix #7307
The type of the column in the schema should be reset after creating an aggregate function descriptor through the function
aggregation.NewAggFuncDesc()What is changed and how it works?
Check List
Tests
Related changes