Copilot commented on code in PR #12163: URL: https://github.com/apache/gluten/pull/12163#discussion_r3371307220
########## 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_05 -VELOX_ENHANCED_BRANCH=ibm-2026_06_05 +VELOX_REPO=https://github.com/JkSelf/velox.git +VELOX_BRANCH=dft-2026_06_05-hashtable-cache +VELOX_ENHANCED_BRANCH=ibm-2026_06_05-hashtable-cache Review Comment: The default Velox source is switched to a personal fork/branch. For reproducible builds and supply-chain safety, the default should remain an official upstream (or org-controlled) repository; developers can still override via --velox_repo/--velox_branch when testing. ########## cpp/velox/compute/WholeStageResultIterator.cc: ########## @@ -72,6 +72,19 @@ 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_Stage_{}_TID_{}_VTID_{}", + std::to_string(taskInfo.stageId), + std::to_string(taskInfo.taskId), + std::to_string(taskInfo.vId)); +} Review Comment: Using only Spark SQL executionId as the Velox Task id will be identical across all Spark tasks belonging to the same SQL execution. Velox Task ids are typically required to be unique per running task; collisions can lead to task registry conflicts or cross-task state bleed. Consider keeping per-task uniqueness and (optionally) prefixing with executionId for grouping. ########## cpp/velox/substrait/SubstraitToVeloxPlan.cc: ########## @@ -448,29 +448,11 @@ 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(), + joinNodeId, Review Comment: `joinNodeId` is set to `hashtableid` when provided. If `hashtableid` is a numeric SparkPlan id (e.g., "5"), it can collide with ids produced by `nextPlanNodeId()` (which are sequential numeric strings starting from 0), violating Velox's expectation that PlanNodeIds are unique within a plan. This needs a namespacing strategy (e.g., keep a non-numeric prefix like the previous "BuiltHashTable-", or prefix all `nextPlanNodeId()` ids) that is consistent with the HashTableCache key used at build time. ########## gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala: ########## @@ -392,7 +403,7 @@ abstract class BroadcastHashJoinExecTransformerBase( override def hashJoinType: JoinType = joinType // Unique ID for builded hash table - lazy val buildHashTableId: String = "BuiltHashTable-" + buildPlan.id + lazy val buildHashTableId: String = canonicalBuildHashTableId(buildPlan) Review Comment: `buildHashTableId` now resolves to the underlying SparkPlan numeric id. This id is propagated into Substrait and then reused as a Velox PlanNodeId / cache key, where purely-numeric ids can collide with other generated plan node ids (see SubstraitToVeloxPlanConverter::nextPlanNodeId()) and are also not obviously namespaced. Consider restoring a stable, non-numeric prefix (e.g., "BuiltHashTable-") while still canonicalizing through BroadcastQueryStageExec/ReusedExchangeExec. ########## backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala: ########## @@ -106,7 +106,7 @@ case class BroadcastHashJoinExecTransformer( isNullAwareAntiJoin) { // Unique ID for built table - lazy val buildBroadcastTableId: String = buildPlan.id.toString + lazy val buildBroadcastTableId: String = canonicalBuildHashTableId(buildPlan) Review Comment: `buildBroadcastTableId` is now the canonical SparkPlan numeric id, which is later used as the broadcast hash table id/cache key. As with the Substrait-side usage, purely-numeric ids can collide with other Velox plan node ids and are not namespaced. Consider using a stable prefixed form (e.g., "BuiltHashTable-<canonicalId>") while still canonicalizing through BroadcastQueryStageExec/ReusedExchangeExec. -- 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]
