Copilot commented on code in PR #12487:
URL: https://github.com/apache/gluten/pull/12487#discussion_r3557036827
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -469,11 +484,11 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
leftNode,
rightNode,
getJoinOutputType(leftNode, rightNode, joinType),
- true,
+ useHashTableCache,
false,
false,
nullptr,
- sJoin.hashtableid());
+ hashTableId);
Review Comment:
The PR title/description talk about releasing hash tables at
SQLExecutionEnd, but this change actually gates whether HashJoinNode is
configured to use the broadcast hash table cache based on whether a pre-built
HashTableBuilder can be retrieved. If the intent is primarily to avoid leaking
cached hash tables, consider updating the PR title/description (or adding the
missing cleanup changes) so reviewers/operators understand the actual mechanism
being changed.
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -469,11 +484,11 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
leftNode,
rightNode,
getJoinOutputType(leftNode, rightNode, joinType),
- true,
+ useHashTableCache,
false,
false,
nullptr,
- sJoin.hashtableid());
+ hashTableId);
Review Comment:
This introduces new behavior for BHJ plan conversion (enabling/disabling
hash table cache depending on whether a pre-built HashTableBuilder is present).
There doesn’t appear to be a regression test exercising this branch (e.g.,
cache hit vs miss) to ensure correctness and to prevent the original
memory-leak/cleanup issue from reappearing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
