-
Notifications
You must be signed in to change notification settings - Fork 409
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
Split AstToExecutor from dbgFuncCoprocessor. #4610
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
@ywqzzy: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
dbms/src/Debug/AstToExecutor.cpp
Outdated
} | ||
else if (ASTFunction * func = typeid_cast<ASTFunction *>(ast.get())) | ||
{ | ||
/// aggregation function is handled in Aggregation, so just treated as a column |
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.
how about move this branch into another function?
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.
how about move this branch into another function?
Good idea. I will refactor them all.
dbms/src/Debug/AstToExecutor.cpp
Outdated
} | ||
else if (ASTFunction * func = typeid_cast<ASTFunction *>(ast.get())) | ||
{ | ||
/// check function |
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.
ditto
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.
done
dbms/src/Debug/AstToExecutor.cpp
Outdated
} | ||
} // namespace | ||
|
||
String LOCAL_HOST = "127.0.0.1:3930"; |
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.
move to anonymous namespace?
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.
done. I have moved it into namespace db::debug.
dbms/src/Debug/AstToExecutor.h
Outdated
} | ||
virtual bool toTiPBExecutor(tipb::Executor * tipb_executor, uint32_t collator_id, const MPPInfo & mpp_info, const Context & context) | ||
= 0; | ||
virtual void toMPPSubPlan(size_t & executor_index, const DAGProperties & properties, std::unordered_map<String, std::pair<std::shared_ptr<ExchangeReceiver>, std::shared_ptr<ExchangeSender>>> & exchange_map) |
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.
is executor_index
expected to be mutable to derived classes of Executor
?
dbms/src/Debug/AstToExecutor.h
Outdated
tipb::ExchangeType type; | ||
TaskMetas task_metas; | ||
std::vector<size_t> partition_keys; | ||
ExchangeSender(size_t & index, const DAGSchema & output, tipb::ExchangeType type_, const std::vector<size_t> & partition_keys_ = {}) |
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.
is index
mutable?
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.
it is mutable
dbms/src/Debug/AstToExecutor.h
Outdated
struct ExchangeReceiver : Executor | ||
{ | ||
TaskMetas task_metas; | ||
ExchangeReceiver(size_t & index, const DAGSchema & output) |
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.
ditto
dbms/src/Debug/AstToExecutor.h
Outdated
{ | ||
TableInfo table_info; | ||
/// used by column pruner | ||
TableScan(size_t & index_, const DAGSchema & output_schema_, TableInfo & table_info_) |
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.
ditto, the same as table_info_
.
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.
done
dbms/src/Debug/AstToExecutor.h
Outdated
struct Selection : public Executor | ||
{ | ||
std::vector<ASTPtr> conditions; | ||
Selection(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> && conditions_) |
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.
Selection(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> && conditions_) | |
Selection(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> conditions_) |
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.
done.
dbms/src/Debug/AstToExecutor.h
Outdated
{ | ||
std::vector<ASTPtr> order_columns; | ||
size_t limit; | ||
TopN(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> && order_columns_, size_t limit_) |
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.
TopN(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> && order_columns_, size_t limit_) | |
TopN(size_t & index_, const DAGSchema & output_schema_, std::vector<ASTPtr> order_columns_, size_t limit_) |
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.
done
fbb50e5
to
0704608
Compare
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
/run-integration-test |
/cc @yibin87 |
/run-all-tests |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
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
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.
Others LGTM
dbms/src/Debug/astToExecutor.cpp
Outdated
{"group_concat", tipb::ExprType::GroupConcat}, | ||
}); | ||
|
||
DAGColumnInfo toNullableDAGColumnInfo(DAGColumnInfo & input) |
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.
Make the parameter const
?
4035bb4
to
f8a5692
Compare
/merge |
@ywqzzy: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: f8a5692
|
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
What problem does this PR solve?
Issue Number: ref #4609
Problem Summary:
Need to provide Ast to tipb::Executor helper functions for Interpreter test framework, but all of them are in .cpp file.
What is changed and how it works?
Move Ast to tipb::Executor helper functions to astToExecutor.h
Check List
Tests
Side effects
Documentation
Release note