Copilot commented on code in PR #12163: URL: https://github.com/apache/gluten/pull/12163#discussion_r3401889370
########## ep/build-velox/src/get-velox.sh: ########## @@ -17,9 +17,9 @@ set -exu CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd) -VELOX_REPO=https://github.com/IBM/velox.git -VELOX_BRANCH=dft-2026_06_06 -VELOX_ENHANCED_BRANCH=ibm-2026_06_06 +VELOX_REPO=https://github.com/JkSelf/velox.git +VELOX_BRANCH=dft-2026_06_06-hashtable-cache +VELOX_ENHANCED_BRANCH=ibm-2026_06_06-hashtable-cache Review Comment: Defaulting the Velox checkout to a personal fork/branch makes builds non-reproducible and introduces supply-chain risk. The default should point to the project’s canonical Velox repo (or a pinned commit in an approved upstream fork); developers can still override via --velox_repo/--velox_branch for testing. ########## gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala: ########## @@ -105,6 +107,15 @@ trait HashJoinLikeExecTransformer extends BaseJoinExec with TransformSupport { (right, left) } + protected def canonicalBuildHashTableId(plan: SparkPlan): String = plan match { + case b: BroadcastQueryStageExec => + canonicalBuildHashTableId(b.plan) + case r: ReusedExchangeExec => + canonicalBuildHashTableId(r.child) + case other => + other.id.toString Review Comment: canonicalBuildHashTableId currently returns a plain numeric SparkPlan id. This id is later used as the Velox HashJoinNode PlanNodeId (see cpp/velox/substrait/SubstraitToVeloxPlan.cc), which can collide with other PlanNodeIds generated by nextPlanNodeId() (also numeric). Prefixing avoids accidental PlanNodeId collisions while keeping canonicalization for reuse/exchange. ########## cpp/velox/jni/VeloxJniWrapper.cc: ########## @@ -1060,6 +1061,13 @@ 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->hasTable(hashTableId)) { + std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n"; Review Comment: Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message. ########## cpp/velox/jni/VeloxJniWrapper.cc: ########## @@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native } hashTableBuilders[0]->setHashTable(std::move(mainTable)); + + auto* cache = facebook::velox::exec::HashTableCache::instance(); + if (!cache->hasTable(hashTableId)) { + std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n"; + cache->injectTable( + hashTableId, + hashTableBuilders[0]->hashTable(), + hashTableBuilders[0]->joinHasNullKeys(), + defaultLeafVeloxMemoryPool()); + } + return gluten::getHashTableObjStore()->save(hashTableBuilders[0]); JNI_METHOD_END(kInvalidObjectHandle) } JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_cloneHashTable( // NOLINT JNIEnv* env, jclass, + jstring cacheKey, jlong tableHandler) { JNI_METHOD_START + auto cacheKeyStr = jStringToCString(env, cacheKey); auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler); + auto* cache = facebook::velox::exec::HashTableCache::instance(); + if (!cache->hasTable(cacheKeyStr)) { + cache->injectTable( + cacheKeyStr, hashTableHandler->hashTable(), hashTableHandler->joinHasNullKeys(), defaultLeafVeloxMemoryPool()); + } + return gluten::getHashTableObjStore()->save(hashTableHandler); JNI_METHOD_END(kInvalidObjectHandle) } JNIEXPORT void JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_clearHashTable( // NOLINT JNIEnv* env, jclass, + jstring cacheKey, jlong tableHandler) { JNI_METHOD_START - auto hashTableHandler = ObjectStore::retrieve<gluten::HashTableBuilder>(tableHandler); - hashTableHandler->hashTable()->clear(true); + auto cacheKeyStr = jStringToCString(env, cacheKey); + std::cout << "VeloxJniWrapper clear hash table id is " << cacheKeyStr << "\n"; Review Comment: Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message. ########## cpp/velox/compute/WholeStageResultIterator.cc: ########## @@ -72,6 +72,15 @@ const std::string kHiveDefaultPartition = "__HIVE_DEFAULT_PARTITION__"; } // namespace +namespace { +std::string getVeloxTaskId(const SparkTaskInfo& taskInfo) { + if (taskInfo.executionId != -1) { + return fmt::format("Gluten_Execution_{}", std::to_string(taskInfo.executionId)); + } + return fmt::format("Gluten_Execution_{}", ""); +} Review Comment: If Spark execution id is unavailable, getVeloxTaskId currently returns the same constant string ("Gluten_Execution_") for all tasks. That can cause unrelated queries/tasks to share the same Velox QueryCtx id and potentially collide in HashTableCache scope. Use a unique fallback derived from stage/task/virtual id instead of an empty suffix. ########## backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala: ########## @@ -135,9 +135,12 @@ case class BroadcastHashJoinExecTransformer( override def columnarInputRDDs: Seq[RDD[ColumnarBatch]] = { val streamedRDD = getColumnarInputRDDs(streamedPlan) val executionId = sparkContext.getLocalProperty(SQLExecution.EXECUTION_ID_KEY) + var cacheKey = "" if (executionId != null) { - GlutenDriverEndpoint.collectResources(executionId, buildBroadcastTableId) + cacheKey = "Gluten_Execution_" + executionId + ":" + buildHashTableId + GlutenDriverEndpoint.collectResources(executionId, cacheKey) Review Comment: BroadcastHashJoinContext.buildHashTableId is used as the native hash table/cache identifier (passed into HashJoinBuilder.nativeBuild/clone/clear), but here it’s being rewritten to include the execution id. Meanwhile the Substrait join’s hashtableid is derived from canonicalBuildHashTableId(buildPlan) (without the execution prefix). This key mismatch will prevent cache hits and may also break cleanup because native and JVM caches will be keyed differently. ########## cpp/velox/jni/VeloxJniWrapper.cc: ########## @@ -1138,30 +1146,52 @@ JNIEXPORT jlong JNICALL Java_org_apache_gluten_vectorized_HashJoinBuilder_native } hashTableBuilders[0]->setHashTable(std::move(mainTable)); + + auto* cache = facebook::velox::exec::HashTableCache::instance(); + if (!cache->hasTable(hashTableId)) { + std::cout << "VeloxJniWrapper inject hash table id is " << hashTableId << "\n"; Review Comment: Avoid writing directly to stdout from JNI/native hot paths; it can flood executor logs and is hard to control. Use the existing logging macros (LOG/VLOG) or remove the message. -- 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]
