Copilot commented on code in PR #12309:
URL: https://github.com/apache/gluten/pull/12309#discussion_r3424682289
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/JoinExecTransformer.scala:
##########
@@ -301,9 +301,8 @@ trait HashJoinLikeExecTransformer extends BaseJoinExec with
TransformSupport {
BackendsApiManager.getTransformerApiInstance.packPBMessage(message)
}
- def genJoinParametersInternal(): (Int, Int, String) = {
- (0, 0, "")
- }
+ // isBHJ, isNullAwareAntiJoin, buildTableId.
+ def genJoinParametersInternal(): (Int, Int, Int)
Review Comment:
`HashJoinLikeExecTransformer` now declares `genJoinParametersInternal` as
abstract, but only a couple of subclasses implement it (search shows many join
transformers still rely on the old default). This will break compilation for
any class extending `HashJoinLikeExecTransformer` without an override. Consider
restoring a safe default implementation (e.g., SHJ defaults) so existing
subclasses keep compiling, and override where needed.
##########
backends-clickhouse/src/main/scala/org/apache/spark/sql/execution/joins/ClickHouseBuildSideRelation.scala:
##########
@@ -74,10 +73,10 @@ case class ClickHouseBuildSideRelation(
existBroadCastHashJoinContext = null
}
- private def couldReuseHashTableData(broadCastContext:
BroadCastHashJoinContext): Boolean = {
+ private def couldReuseHashTableData(broadcastContext:
BroadCastHashJoinContext): Boolean = {
Review Comment:
`couldReuseHashTableData` still takes `BroadCastHashJoinContext`, but
callers now pass `BroadcastJoinContext`. This is a type mismatch that will fail
compilation.
##########
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=")
Review Comment:
The native side parses `buildHashTableId` (see
`AdvancedParametersParseUtil.cpp`: `tryAssign(kvs, "buildHashTableId", ...)`).
Emitting `buildBroadcastTableId` here means the broadcast build table ID won’t
be parsed, so the storage join can’t be looked up.
##########
cpp-ch/local-engine/Parser/AdvancedParametersParseUtil.h:
##########
@@ -34,7 +34,7 @@ struct JoinOptimizationInfo
Int64 right_table_rows = -1;
Int64 right_table_bytes = -1;
Int64 partitions_num = -1;
- String storage_join_key;
+ Int32 storage_join_key = -1;
Review Comment:
`storage_join_key` was changed to `Int32`, but `JoinOptimizationInfo::parse`
assigns it via `tryAssign(kvs, "buildHashTableId", info.storage_join_key)` in
`AdvancedParametersParseUtil.cpp`. `tryAssign` has specializations for
`String`, `bool`, and `Int64` only; there is no `Int32` specialization, and the
generic template is only forward-declared, so this will fail to link/compile.
Add a `tryAssign<Int32>` specialization (or switch this field to `Int64`
consistently).
##########
backends-clickhouse/src/main/scala/org/apache/spark/sql/execution/joins/ClickHouseBuildSideRelation.scala:
##########
@@ -47,22 +47,21 @@ case class ClickHouseBuildSideRelation(
private var existHashTableData: Long = 0L
private var existBroadCastHashJoinContext: BroadCastHashJoinContext = null
Review Comment:
`existBroadCastHashJoinContext` is still typed as
`BroadCastHashJoinContext`, but this file now imports/uses
`BroadcastJoinContext`. As-is, this won’t compile and also prevents correct
cache reuse checks.
##########
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")
Review Comment:
For CH broadcast nested-loop join, `isBHJ` is used by the native parser
(`JoinOptimizationInfo::parse`) to decide whether to fetch the prebuilt
broadcast join from `CHBroadcastBuildSideCache` (see `CrossRelParser` /
`JoinRelParser`). Setting it to `0` will prevent retrieving the storage join;
since the build-side RDD is side-effect-only (returns empty), the join would
execute without the broadcast build table.
##########
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) =
Review Comment:
The Caffeine cache key type is now `Int`, but the mapping function passed to
`Cache.get` still declares the key as `String`. This won’t compile and also
makes the log message misleading (it’s an Int ID now).
##########
cpp-ch/local-engine/local_engine_jni.cpp:
##########
@@ -1167,11 +1168,10 @@ JNIEXPORT jlong
Java_org_apache_gluten_vectorized_StorageJoinBuilder_nativeClone
}
JNIEXPORT void
-Java_org_apache_gluten_vectorized_StorageJoinBuilder_nativeCleanBuildHashTable(JNIEnv
* env, jclass, jstring hash_table_id_, jlong instance)
+Java_org_apache_gluten_vectorized_StorageJoinBuilder_nativeCleanBuildHashTable(JNIEnv
* env, jclass, jint hash_table_id_, jlong instance)
{
LOCAL_ENGINE_JNI_METHOD_START
- auto hash_table_id = jstring2string(env, hash_table_id_);
- local_engine::BroadCastJoinBuilder::cleanBuildHashTable(hash_table_id,
instance);
+
local_engine::BroadcastJoinBuilder::cleanBuildHashTable((int)hash_table_id,
instance);
LOCAL_ENGINE_JNI_METHOD_END(env, )
Review Comment:
`nativeCleanBuildHashTable` now takes a `jint hash_table_id_`, but this
implementation still refers to `hash_table_id` (undefined). This is a compile
error.
--
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]
