From 327dbcf35de99f2675d70ca0bf69fedaaffc2aa2 Mon Sep 17 00:00:00 2001 From: Mohammad Linjawi Date: Tue, 21 Jul 2026 17:23:21 +0300 Subject: [PATCH] [GLUTEN-12504][VL] Fix segfault looking up BHJ hash table without a JVM getJoin() dereferenced JniHashTableContext::vm_ unconditionally. vm_ is only set from JNI_OnLoad, so in a process with no JVM attached (the standalone micro benchmark, the native tests) it is null and converting any substrait plan with a non-empty hashTableId crashed. The existing try/catch at the call site cannot help, since a null deref is a signal rather than an exception. Report a cache miss instead, so the caller builds the hash table from the join's build side input. Also add the missing checkException/DeleteLocalRef in callJavaGet, matching the other JNI callbacks in this tree. Co-Authored-By: Claude Opus 4.8 --- cpp/velox/jni/JniHashTable.cc | 17 ++++++++- cpp/velox/jni/JniHashTable.h | 8 +++++ cpp/velox/tests/CMakeLists.txt | 1 + cpp/velox/tests/JniHashTableTest.cc | 53 +++++++++++++++++++++++++++++ docs/developers/MicroBenchmarks.md | 13 +++++-- 5 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 cpp/velox/tests/JniHashTableTest.cc diff --git a/cpp/velox/jni/JniHashTable.cc b/cpp/velox/jni/JniHashTable.cc index d7259879c59..05545f0bc26 100644 --- a/cpp/velox/jni/JniHashTable.cc +++ b/cpp/velox/jni/JniHashTable.cc @@ -46,6 +46,10 @@ void JniHashTableContext::finalize(JNIEnv* env) { } jlong JniHashTableContext::callJavaGet(const std::string& id) const { + if (vm_ == nullptr) { + throw gluten::GlutenException("No JVM attached to this process"); + } + JNIEnv* env; if (vm_->GetEnv(reinterpret_cast(&env), jniVersion) != JNI_OK) { throw gluten::GlutenException("JNIEnv was not attached to current thread"); @@ -53,6 +57,8 @@ jlong JniHashTableContext::callJavaGet(const std::string& id) const { const jstring s = env->NewStringUTF(id.c_str()); auto result = env->CallStaticLongMethod(jniVeloxBroadcastBuildSideCache_, jniGet_, s); + env->DeleteLocalRef(s); + checkException(env); return result; } @@ -161,7 +167,16 @@ std::shared_ptr nativeHashTableBuild( } long getJoin(const std::string& hashTableId) { - return JniHashTableContext::getInstance().callJavaGet(hashTableId); + const auto& context = JniHashTableContext::getInstance(); + if (!context.hasJavaVM()) { + // No JVM is attached to this process, e.g. the standalone micro benchmark, so the JVM side + // VeloxBroadcastBuildSideCache holding the pre-built hash tables is unreachable. Report a miss + // and let the caller build the hash table from the join's build side input instead. + LOG(WARNING) << "No JVM is attached to this process, cannot look up the pre-built hash table for cache key: " + << hashTableId << ". A new hash table will be built from the build side input."; + return 0; + } + return context.callJavaGet(hashTableId); } size_t serializedHashTableSize(std::shared_ptr builder) { diff --git a/cpp/velox/jni/JniHashTable.h b/cpp/velox/jni/JniHashTable.h index d75d2d663bc..69c2cd06c21 100644 --- a/cpp/velox/jni/JniHashTable.h +++ b/cpp/velox/jni/JniHashTable.h @@ -48,6 +48,12 @@ class JniHashTableContext { return vm_; } + // Returns false when no JVM is attached to this process, which is the case for the standalone + // micro benchmark and the native unit tests. Callers must not attempt any JNI call then. + bool hasJavaVM() const { + return vm_ != nullptr; + } + ObjectStore* getHashTableObjStore() const { return hashTableObjStore_.get(); } @@ -89,6 +95,8 @@ std::shared_ptr nativeHashTableBuild( std::vector>& batches, std::shared_ptr memoryPool); +// Returns the handle of the pre-built hash table registered for the given id, or 0 if there is +// none. Safe to call from a process with no JVM attached, in which case it always returns 0. long getJoin(const std::string& hashTableId); // Return the exact serialized hash table size for direct buffer allocation. diff --git a/cpp/velox/tests/CMakeLists.txt b/cpp/velox/tests/CMakeLists.txt index 87331de818c..c08615f5541 100644 --- a/cpp/velox/tests/CMakeLists.txt +++ b/cpp/velox/tests/CMakeLists.txt @@ -142,6 +142,7 @@ if(ENABLE_S3) endif() add_velox_test(scoped_timer_test SOURCES ScopedTimerTest.cc) add_velox_test(row_based_checksum_test SOURCES RowBasedChecksumTest.cc) +add_velox_test(jni_hash_table_test SOURCES JniHashTableTest.cc) if(BUILD_EXAMPLES) add_velox_test(my_udf_test SOURCES MyUdfTest.cc) endif() diff --git a/cpp/velox/tests/JniHashTableTest.cc b/cpp/velox/tests/JniHashTableTest.cc new file mode 100644 index 00000000000..37792de558b --- /dev/null +++ b/cpp/velox/tests/JniHashTableTest.cc @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "jni/JniHashTable.h" + +#include + +#include "utils/Exception.h" + +namespace gluten { + +// This test binary, like the micro benchmark, runs without a JVM. JNI_OnLoad is therefore never +// invoked and JniHashTableContext::vm_ stays null. +// +// Looking up the build side of a broadcast hash join used to dereference that null JavaVM +// unconditionally, so converting any substrait plan carrying a non-empty `hashTableId` crashed the +// process. See https://github.com/apache/gluten/issues/12504. +class JniHashTableTest : public ::testing::Test { + protected: + void SetUp() override { + ASSERT_FALSE(JniHashTableContext::getInstance().hasJavaVM()) + << "This test asserts behaviour of a process with no JVM attached"; + } +}; + +TEST_F(JniHashTableTest, getJoinReportsMissWithoutJvm) { + // Must report a miss rather than segfault. SubstraitToVeloxPlanConverter treats 0 as "no + // pre-built table" and falls back to building one from the build side input. + EXPECT_EQ(getJoin("no-such-broadcast-hash-table-id"), 0); +} + +TEST_F(JniHashTableTest, callJavaGetThrowsWithoutJvm) { + // The low level accessor reports the missing JVM as a catchable error. GlutenException derives + // from std::runtime_error, so the catch(const std::exception&) around the call site in + // SubstraitToVeloxPlan.cc handles it. + EXPECT_THROW(JniHashTableContext::getInstance().callJavaGet("any-id"), GlutenException); +} + +} // namespace gluten diff --git a/docs/developers/MicroBenchmarks.md b/docs/developers/MicroBenchmarks.md index ce0996a6cc2..d394fa5f7ba 100644 --- a/docs/developers/MicroBenchmarks.md +++ b/docs/developers/MicroBenchmarks.md @@ -138,8 +138,7 @@ or 4 types of dumped file: be more than one split file in a first stage task. Contains the substrait plan piece to the input file splits. - Data file(optional): Parquet formatted, file - name `data_{stageId}_{partitionId}_{vId}_{iteratorIdx}.parquet`. If the first stage contains one or - more BHJ operators, there can be one or more input data files. The input data files of a first + name `data_{stageId}_{partitionId}_{vId}_{iteratorIdx}.parquet`. The input data files of a first stage will be loaded as iterators to serve as the inputs for the pipeline: ``` @@ -152,6 +151,16 @@ or 4 types of dumped file: } ``` +### Limitation: broadcast hash join + +A stage containing a broadcast hash join cannot currently be replayed faithfully. The build side of +a BHJ is not streamed to the native operator: the hash table is built on the driver, broadcast in +serialized form, and registered in a process-local cache that the standalone benchmark has no access +to. The corresponding build side input iterator yields no rows, so its dumped +`data_{stageId}_{partitionId}_{vId}_{iteratorIdx}.parquet` is empty and replaying the stage produces +no join output. The benchmark logs a warning that no JVM is attached and that a new hash table will +be built from the (empty) build side input. + Run benchmark. By default, the result will be printed to stdout. You can use `--noprint-result` to suppress this output.