Copilot commented on code in PR #12163:
URL: https://github.com/apache/gluten/pull/12163#discussion_r3517867295
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -1048,6 +1049,12 @@ JNIEXPORT jlong JNICALL
Java_org_apache_gluten_vectorized_HashJoinBuilder_native
nullptr);
builder->setHashTable(std::move(mainTable));
+ auto* cache = facebook::velox::exec::HashTableCache::instance();
+
+ if (!cache->exist(hashTableId)) {
+ cache->add(hashTableId, builder->hashTable(),
builder->joinHasNullKeys(), defaultLeafVeloxMemoryPool());
+ }
Review Comment:
`hashTableId` is used for `HashTableCache` lookups, but there is no
definition in this function (the `nativeBuild` parameter is currently unnamed
as `jstring /*tableId*/`). As-is, this will not compile; it likely needs to
derive the cache key from the Java `buildHashTableId` string parameter (e.g.,
name the parameter and call `jStringToCString`).
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -458,26 +458,8 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
} else if (
sJoin.has_advanced_extension() &&
SubstraitParser::configSetInOptimization(sJoin.advanced_extension(),
"isBHJ=")) {
- std::string hashTableId = sJoin.hashtableid();
-
- std::shared_ptr<core::OpaqueHashTable> opaqueSharedHashTable = nullptr;
- bool joinHasNullKeys = false;
-
- try {
- auto hashTableBuilder =
ObjectStore::retrieve<gluten::HashTableBuilder>(getJoin(hashTableId));
- joinHasNullKeys = hashTableBuilder->joinHasNullKeys();
- auto originalShared = hashTableBuilder->hashTable();
- opaqueSharedHashTable = std::shared_ptr<core::OpaqueHashTable>(
- originalShared,
reinterpret_cast<core::OpaqueHashTable*>(originalShared.get()));
-
- LOG(INFO) << "Successfully retrieved and aliased HashTable for reuse.
ID: " << hashTableId;
- } catch (const std::exception& e) {
- LOG(WARNING)
- << "Error retrieving HashTable from ObjectStore: " << e.what()
- << ". Falling back to building new table. To ensure correct results,
please verify that spark.gluten.velox.buildHashTableOncePerExecutor.enabled is
set to false.";
- opaqueSharedHashTable = nullptr;
- }
-
+ const auto& hashTableId = sJoin.hashtableid();
+ const auto joinNodeId = hashTableId.empty() ? nextPlanNodeId() :
hashTableId;
// Create HashJoinNode node
return std::make_shared<core::HashJoinNode>(
nextPlanNodeId(),
Review Comment:
`joinNodeId` is declared but never used. With common builds using `-Werror`,
this unused variable will break compilation; it also duplicates the
`hashtableid()` access unnecessarily.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -394,7 +405,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: "builded" should be "built" in this comment.
--
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]
