You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2025/09/22 08:47:54 UTC
[PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
beliefer opened a new pull request, #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771
## What changes are proposed in this pull request?
This PR propose to improve perf for `toVeloxPlan` with `ProjectRel`.
Fixes #10770
## How was this patch tested?
GA tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#issuecomment-3317968306
ping @rui-mo @zuochunwei cc @zhztheplayer @PHILO-HE @zml1206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#issuecomment-3317624552
https://github.com/apache/incubator-gluten/issues/10770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2371643592
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770][VL] Improve std::vector usage in `toVeloxPlan` [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#issuecomment-3326262288
@PHILO-HE @zhztheplayer Thank you all!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2369258428
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review Comment:
Curious, does the new code improve performance in real workloads?
These changes themselves feel reasonable to me though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770][VL] Improve std::vector usage in `toVeloxPlan` [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE merged PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2370834339
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2371661872
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review Comment:
Got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Code cleanup for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2371661872
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review Comment:
Got it. The title updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-10770] Improve perf for toVeloxPlan with ProjectRel [incubator-gluten]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #10771:
URL: https://github.com/apache/incubator-gluten/pull/10771#discussion_r2370835335
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -475,27 +475,30 @@ core::PlanNodePtr SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
const auto& fieldName = inputType->nameOf(idx);
projectNames.emplace_back(fieldName);
expressions.emplace_back(std::make_shared<core::FieldAccessTypedExpr>(inputType->childAt(idx), fieldName));
- colIdx += 1;
}
// Then, adding project expression related project names and expressions.
- for (const auto& expr : projectExprs) {
- expressions.emplace_back(exprConverter_->toVeloxExpr(expr, inputType));
- projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, colIdx));
- colIdx += 1;
+ const size_t startIdx = expressions.size();
+ for (int i = 0; i < projectExprs.size(); i++) {
+ expressions.emplace_back(exprConverter_->toVeloxExpr(projectExprs[i], inputType));
+ projectNames.emplace_back(SubstraitParser::makeNodeName(planNodeId_, startIdx + i));
}
if (projectRel.has_common()) {
auto relCommon = projectRel.common();
const auto& emit = relCommon.emit();
int emitSize = emit.output_mapping_size();
- std::vector<std::string> emitProjectNames(emitSize);
- std::vector<core::TypedExprPtr> emitExpressions(emitSize);
+ 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]));
Review Comment:
TBH, I'm new to C++. But I got these information after I learned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org