Introduce new execution sequence builder and work unit builder #127
Conversation
ienkovich
force-pushed
the
ienkovich/query-exec-plan
branch
from
197e78a to
21442ff
Compare
|
@alexbaden @kurapov-peter current CI failures are not related to PR, it's modin/numpy version issue and the known flaky test on CUDA. So, this PR is fully ready for review. |
Signed-off-by: ienkovich <[email protected]>
Signed-off-by: ienkovich <[email protected]>
ienkovich
force-pushed
the
ienkovich/query-exec-plan
branch
from
09e00bd to
be73e14
Compare
Signed-off-by: ienkovich <[email protected]>
| ExprPtrVector col_input_exprs, | ||
| ExprPtrVector table_func_input_exprs, | ||
| std::vector<TargetMetaInfo> tuple_type) | ||
| : Node(std::move(inputs)) | ||
| , function_name_(function_name) | ||
| , fields_(fields) | ||
| , fields_(std::move(fields)) |
There was a problem hiding this comment.
Does this change copy -> move all the way through from the constructor?
There was a problem hiding this comment.
I prefer plain object instead of a constant reference for vector args in constructors. This is because const reference always means we create a copy, and plain object means we can create it via either copy or move on a call site. Many of such places were modified during IR refactoring, but some remained.
I see there are still some similar cases left. I think I'll go through Node constructors and make them all in a separate PR.
This is an updated version of https://github.com/intel-ai/omniscidb/pull/576. This time changes are ready for review and merge. Please see the original PR for more details on the proposed changes.