-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[opt](nereids) infer result column name in ctas and query stmt #26055
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
[opt](nereids) infer result column name in ctas and query stmt #26055
Conversation
…nd doesn't alias artificially when create or select stmt in nereids
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
zddr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
PR approved by anyone and no changes requested. |
| .collect(ImmutableList.toImmutableList()); | ||
| return new LogicalResultSink<>(outputExprs, sink.child()); | ||
|
|
||
| final ImmutableListMultimap.Builder<ExprId, Integer> exprIdToIndexMapBuilder = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not see how to process ctas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ctas stmt is supported in nereids now and it's also call the method org.apache.doris.nereids.NereidsPlanner#plan, this pr is also useful. the old and new optimizer should keep consistency when query and ddl
| /** | ||
| * Infer output column name when it refers an expression and not has an alias manually. | ||
| */ | ||
| public static class InferPlanOutputAlias extends DefaultPlanVisitor<Void, ImmutableMultimap<ExprId, Integer>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one vistor in on file under dir visitor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
| @Override | ||
| public Void visit(Plan plan, ImmutableMultimap<ExprId, Integer> context) { | ||
|
|
||
| List<NamedExpression> projects = plan.getExpressions().stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getExpressions is not use for get projects. what do u want to get?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to get all node output expressions and check if it's in the currentExprIdAndIndex Map. if contains then infer the name.
I see that getExpressions is the superset to getProjects, so I use the getExpressions
| } | ||
|
|
||
| @Override | ||
| public Void visit(Plan plan, ImmutableMultimap<ExprId, Integer> context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
context is not a good name, use a meaningful name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, rename it tocurrentExprIdAndIndexMap.
| // Infer name when alias child is expression and alias's name is from child | ||
| if (currentOutputExprIdSet.contains(projectItem.getExprId()) | ||
| && projectItem instanceof Alias | ||
| && ((Alias) projectItem).isNameFromChild()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use isNameFromChild, it will be removed in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alias construct logical as following:
public Expression visitUnboundAlias(UnboundAlias unboundAlias, CascadesContext context) {
Expression child = unboundAlias.child().accept(this, context);
if (unboundAlias.getAlias().isPresent()) {
return new Alias(child, unboundAlias.getAlias().get());
} else if (child instanceof NamedExpression) {
return new Alias(child, ((NamedExpression) child).getName());
} else {
return new Alias(child);
}
}
if the alias name is set by child.toSql() we should infer the alias name. isNameFromChild field identify the name is from child. Maybe we should add anther field to record the info. WDYT?
| ExprId exprId = projectItem.getExprId(); | ||
| // Infer name when alias child is expression and alias's name is from child | ||
| if (currentOutputExprIdSet.contains(projectItem.getExprId()) | ||
| && projectItem instanceof Alias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if u only need alias why not just collect alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
|
run buildall |
|
run buildall |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
|
PR approved by at least one committer and no changes requested. |
…e#26055) Infer name if it is an expression and doesn't alias artificially when create or select stmt in nereids. The infer name strategy is the same as apache#24990
Disable infer column name when query, because it cause some errors when using BI tools This feature is firstly developed by #26055
Disable infer column name when query, because it cause some errors when using BI tools This feature is firstly developed by apache#26055
…e#26055) Infer name if it is an expression and doesn't alias artificially when create or select stmt in nereids. The infer name strategy is the same as apache#24990
Disable infer column name when query, because it cause some errors when using BI tools This feature is firstly developed by apache#26055
Proposed changes
Infer name if it is an expression and doesn't alias artificially when create or select stmt in nereids.
The infer name strategy is the same as #24990
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...