Copilot commented on code in PR #12309:
URL: https://github.com/apache/gluten/pull/12309#discussion_r3433168569
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHHashJoinExecTransformer.scala:
##########
@@ -119,7 +119,7 @@ case class CHShuffledHashJoinExecTransformer(
}
override def genJoinParameters(): Any = {
- val (isBHJ, isNullAwareAntiJoin, buildHashTableId): (Int, Int, String) =
(0, 0, "")
+ val (isBHJ, isNullAwareAntiJoin, buildHashTableId): (Int, Int, Int) = (0,
0, 0)
Review Comment:
`HashJoinLikeExecTransformer` now requires `genJoinParametersInternal():
(Int, Int, Int)`, but `CHShuffledHashJoinExecTransformer` doesn’t implement it.
This will make the class fail to compile (it’s concrete). Implement the method
and reuse it in `genJoinParameters()` to avoid duplication.
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHBroadcastNestedLoopJoinExecTransformer.scala:
##########
@@ -98,9 +99,9 @@ case class CHBroadcastNestedLoopJoinExecTransformer(
val joinParametersStr = new StringBuffer("JoinParameters:")
joinParametersStr
.append("isBHJ=")
- .append(1)
+ .append(0)
.append("\n")
- .append("buildHashTableId=")
+ .append("buildBroadcastTableId=")
.append(buildBroadcastTableId)
.append("\n")
Review Comment:
The emitted join parameters use key `buildBroadcastTableId`, but the
ClickHouse native side parses `buildHashTableId` (see
`cpp-ch/local-engine/Parser/AdvancedParametersParseUtil.cpp` where it assigns
`storage_join_key` from `buildHashTableId`). With the current key name, the
storage-join lookup will fail and the broadcast table won’t be found in cache.
##########
cpp/velox/jni/VeloxJniWrapper.cc:
##########
@@ -960,7 +960,7 @@ JNIEXPORT jobject JNICALL
Java_org_apache_gluten_execution_IcebergWriteJniWrappe
JNIEXPORT jlong JNICALL
Java_org_apache_gluten_vectorized_HashJoinBuilder_nativeBuild( // NOLINT
JNIEnv* env,
jobject wrapper,
- jstring tableId,
+ jint tableId,
jlongArray batchHandles,
jobjectArray joinKeys,
jobjectArray filterBuildColumns,
Review Comment:
`tableId` was switched from `jstring` to `jint`, but it is not referenced
anywhere in the function body anymore. With `-Werror` enabled, this commonly
fails the native build due to `-Wunused-parameter`. Either use the value or
explicitly mark it unused (e.g., `(void)tableId;`).
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHBroadcastBuildSideCache.scala:
##########
@@ -45,37 +45,37 @@ object CHBroadcastBuildSideCache extends Logging with
RemovalListener[String, Br
// Use for controlling to build bhj hash table once.
// key: hashtable id, value is hashtable backend pointer(long to string).
- private val buildSideRelationCache: Cache[String, BroadcastHashTable] =
+ private val buildSideRelationCache: Cache[Int, BroadcastHashTable] =
Caffeine.newBuilder
.expireAfterAccess(expiredTime, TimeUnit.SECONDS)
.removalListener(this)
- .build[String, BroadcastHashTable]()
+ .build[Int, BroadcastHashTable]()
def getOrBuildBroadcastHashTable(
broadcast: Broadcast[BuildSideRelation],
- broadCastContext: BroadCastHashJoinContext): BroadcastHashTable = {
+ broadcastContext: BroadcastJoinContext): BroadcastHashTable = {
buildSideRelationCache
.get(
- broadCastContext.buildHashTableId,
- (broadcast_id: String) => {
+ broadcastContext.buildTableId,
+ (broadcastId: String) => {
val (pointer, relation) =
broadcast.value
.asInstanceOf[ClickHouseBuildSideRelation]
- .buildHashTable(broadCastContext)
- logDebug(s"Create bhj $broadcast_id = 0x${pointer.toHexString}")
+ .buildHashTable(broadcastContext)
+ logDebug(s"Create bhj $broadcastId = 0x${pointer.toHexString}")
BroadcastHashTable(pointer, relation)
Review Comment:
`buildSideRelationCache.get(...)` expects a mapping function of type `Int =>
BroadcastHashTable`, but the lambda parameter is typed as `String`, which won’t
compile and also makes the debug log misleading. Use `Int` (or `Integer`)
consistently for the cache key.
--
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]
