Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3526284460
##########
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:
This BHJ branch no longer retrieves the prebuilt hash table from the
ObjectStore (old `getJoin(hashTableId)` path) and instead passes `nullptr` plus
`sJoin.hashtableid()` into `HashJoinNode`, which makes correctness depend on
the Velox `HashTableCache` being populated for that key. However, the executor
deserialization path (`SerializedBroadcastHashTable.deserialize()` ->
`HashJoinBuilder.deserializeHashTableDirect`) currently just calls
`gluten::deserializeHashTable(...)` and returns a builder handle without
inserting into `HashTableCache` (see
`cpp/velox/jni/VeloxJniWrapper.cc:1198-1203`,
`cpp/velox/jni/JniHashTable.cc:201-246`). That means Spark-native broadcast
(driver builds/serializes, executor deserializes) can end up with a missing
cache entry for `hashtableid`.
Consider either (a) extending the deserialize JNI path to add the resulting
hash table into `HashTableCache` using the broadcast/build hash table id, or
(b) adding a fallback here to populate the cache from the existing
`getJoin(hashTableId)` handle when the cache is empty.
##########
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:
Comment typo: "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]
