[GLUTEN-10770][VL] Improve std::vector usage in toVeloxPlan#10771
[GLUTEN-10770][VL] Improve std::vector usage in toVeloxPlan#10771philo-he merged 1 commit intoapache:mainfrom
toVeloxPlan#10771Conversation
|
ping @rui-mo @zuochunwei cc @zhztheplayer @philo-he @zml1206 |
| std::vector<std::string> emitProjectNames; | ||
| std::vector<core::TypedExprPtr> emitExpressions; | ||
| emitProjectNames.reserve(emitSize); | ||
| emitExpressions.reserve(emitSize); | ||
|
|
||
| for (int i = 0; i < emitSize; i++) { | ||
| int32_t mapId = emit.output_mapping(i); | ||
| emitProjectNames[i] = projectNames[mapId]; | ||
| emitExpressions[i] = expressions[mapId]; | ||
| emitProjectNames.emplace_back(std::move(projectNames[mapId])); | ||
| emitExpressions.emplace_back(std::move(expressions[mapId])); |
There was a problem hiding this comment.
Curious, does the new code improve performance in real workloads?
These changes themselves feel reasonable to me though.
There was a problem hiding this comment.
First, below two lines create default obj.
std::vector<std::string> emitProjectNames(emitSize);
std::vector<core::TypedExprPtr> emitExpressions(emitSize);
We should avoid the behavior.
Second, std::move could avoid copy obj.
There was a problem hiding this comment.
TBH, I'm new to C++. But I got these information after I learned.
There was a problem hiding this comment.
Yes, in that case let's rephrase the PR title / description as code cleanup rather than improve perf. It feels this code is not on hot path so let's be conservative until the performance improvement is actually proved.
There was a problem hiding this comment.
Got it. The title updated.
beliefer
changed the title
philo-he
changed the title
toVeloxPlan
philo-he
changed the title
toVeloxPlantoVeloxPlan
|
@philo-he @zhztheplayer Thank you all! |
3 participants
What changes are proposed in this pull request?
This PR propose to improve perf for
toVeloxPlanwithProjectRel.Fixes #10770
How was this patch tested?
GA tests.