Copilot commented on code in PR #13227:
URL: https://github.com/apache/gluten/pull/13227#discussion_r4229235061
##########
cpp/velox/utils/VeloxWholeStageDumper.h:
##########
@@ -28,7 +28,7 @@ class VeloxWholeStageDumper final : public WholeStageDumper {
const SparkTaskInfo& taskInfo,
const std::string& saveDir,
int64_t batchSize,
- facebook::velox::memory::MemoryPool* aggregatePool);
+ VeloxMemoryManager* memoryManager);
Review Comment:
`VeloxWholeStageDumper.h` now exposes `VeloxMemoryManager` in its public
interface, but the diff doesn’t show any include or forward declaration for
`VeloxMemoryManager`. This can break compilation for translation units that
include this header without already including `VeloxMemoryManager.h`. Fix by
adding a forward declaration (`class VeloxMemoryManager;`) in the header
(preferred if only pointers are used), or include the appropriate header.
##########
cpp/velox/utils/VeloxWholeStageDumper.h:
##########
@@ -45,7 +45,10 @@ class VeloxWholeStageDumper final : public WholeStageDumper {
std::string saveDir_;
int64_t batchSize_;
- facebook::velox::memory::MemoryPool* pool_;
+ // Not owned. The memory manager is created and released through JNI
independently of the native Runtime
+ // (see RuntimeJniWrapper), and the Java-side Runtime releases the native
Runtime before the memory manager.
+ // It therefore outlives the dumper and every iterator / batch created by
the Runtime.
Review Comment:
`VeloxWholeStageDumper.h` now exposes `VeloxMemoryManager` in its public
interface, but the diff doesn’t show any include or forward declaration for
`VeloxMemoryManager`. This can break compilation for translation units that
include this header without already including `VeloxMemoryManager.h`. Fix by
adding a forward declaration (`class VeloxMemoryManager;`) in the header
(preferred if only pointers are used), or include the appropriate header.
##########
cpp/velox/tests/VeloxWholeStageDumperTest.cc:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 "utils/VeloxWholeStageDumper.h"
+#include "compute/VeloxBackend.h"
+#include "memory/VeloxColumnarBatch.h"
+#include "memory/VeloxMemoryManager.h"
+#include "velox/vector/tests/utils/VectorTestBase.h"
+
+#include <gtest/gtest.h>
+
+#include <filesystem>
+
+using namespace facebook::velox;
+
+namespace gluten {
+namespace {
+
+// A ColumnarBatchIterator that serves a fixed list of row vectors.
+class RowVectorBatchIterator final : public ColumnarBatchIterator {
+ public:
+ explicit RowVectorBatchIterator(std::vector<RowVectorPtr> vectors) :
vectors_(std::move(vectors)) {}
+
+ std::shared_ptr<ColumnarBatch> next() override {
+ if (idx_ >= vectors_.size()) {
+ return nullptr;
+ }
+ return std::make_shared<VeloxColumnarBatch>(vectors_[idx_++]);
+ }
+
+ private:
+ std::vector<RowVectorPtr> vectors_;
+ size_t idx_{0};
+};
+
+} // namespace
+
+class VeloxWholeStageDumperTest : public ::testing::Test, public
test::VectorTestBase {
+ protected:
+ static void SetUpTestSuite() {
+ VeloxBackend::create(AllocationListener::noop(), {});
+
memory::MemoryManager::testingSetInstance(memory::MemoryManager::Options{});
+ }
+
+ static void TearDownTestSuite() {
+ VeloxBackend::get()->tearDown();
+ }
+
+ void SetUp() override {
+ vmm_ = std::make_unique<VeloxMemoryManager>(
+ kVeloxBackendKind, AllocationListener::noop(),
*VeloxBackend::get()->getBackendConf());
+ saveDir_ = std::filesystem::path(::testing::TempDir()) /
+ fmt::format("gluten-whole-stage-dumper-test-{}",
::testing::UnitTest::GetInstance()->random_seed());
+ std::filesystem::remove_all(saveDir_);
+ }
+
+ void TearDown() override {
+ vmm_.reset();
+ std::filesystem::remove_all(saveDir_);
+ }
Review Comment:
`std::filesystem::remove_all(...)` may throw
`std::filesystem::filesystem_error`. Throwing from test teardown/setup can
terminate the process (depending on exception handling configuration), making
failures harder to diagnose. Prefer the `std::error_code` overload (and
optionally assert/log on error) to keep teardown resilient.
##########
cpp/velox/tests/VeloxWholeStageDumperTest.cc:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 "utils/VeloxWholeStageDumper.h"
+#include "compute/VeloxBackend.h"
+#include "memory/VeloxColumnarBatch.h"
+#include "memory/VeloxMemoryManager.h"
+#include "velox/vector/tests/utils/VectorTestBase.h"
+
+#include <gtest/gtest.h>
+
+#include <filesystem>
+
+using namespace facebook::velox;
+
+namespace gluten {
+namespace {
+
+// A ColumnarBatchIterator that serves a fixed list of row vectors.
+class RowVectorBatchIterator final : public ColumnarBatchIterator {
+ public:
+ explicit RowVectorBatchIterator(std::vector<RowVectorPtr> vectors) :
vectors_(std::move(vectors)) {}
+
+ std::shared_ptr<ColumnarBatch> next() override {
+ if (idx_ >= vectors_.size()) {
+ return nullptr;
+ }
+ return std::make_shared<VeloxColumnarBatch>(vectors_[idx_++]);
+ }
+
+ private:
+ std::vector<RowVectorPtr> vectors_;
+ size_t idx_{0};
+};
+
+} // namespace
+
+class VeloxWholeStageDumperTest : public ::testing::Test, public
test::VectorTestBase {
+ protected:
+ static void SetUpTestSuite() {
+ VeloxBackend::create(AllocationListener::noop(), {});
+
memory::MemoryManager::testingSetInstance(memory::MemoryManager::Options{});
+ }
+
+ static void TearDownTestSuite() {
+ VeloxBackend::get()->tearDown();
+ }
Review Comment:
`memory::MemoryManager::testingSetInstance(...)` modifies a process-global
singleton, but the suite teardown doesn’t restore/reset it. This can cause
cross-test interference when the full test binary runs multiple suites.
Consider saving the previous instance (or using the project’s standard
RAII/utility for test instances) and restoring it in `TearDownTestSuite()`.
--
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]
