Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3541440483
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -489,10 +471,11 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
leftNode,
rightNode,
getJoinOutputType(leftNode, rightNode, joinType),
+ true,
false,
false,
- joinHasNullKeys,
- opaqueSharedHashTable);
+ nullptr,
+ sJoin.hashtableid());
Review Comment:
`joinNodeId` is computed but never used, and the current code calls
`nextPlanNodeId()` twice when `hashtableid` is empty (once for `joinNodeId`,
once for the actual HashJoinNode). This can (a) fail compilation under
`-Werror=unused-variable`, (b) skip/consume a plan node id unexpectedly, and
(c) still pass an empty cache key (`sJoin.hashtableid()`) into the
HashJoinNode, which risks cache-key collisions if HashTableCache is used.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -397,7 +408,8 @@ abstract class BroadcastHashJoinExecTransformerBase(
override def joinBuildSide: BuildSide = buildSide
override def hashJoinType: JoinType = joinType
- lazy val buildHashTableId: String = buildPlan.id.toString
+ // Unique ID for builded hash table
Review Comment:
Spelling in comment: "builded" should be "built".
--
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]
