Address comment

zanmato1984 · May 2, 2024 · 48fe58a · 48fe58a
1 parent 91cf8c8
commit 48fe58a
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 37 deletions.
diff --git a/cpp/src/gandiva/cache.cc b/cpp/src/gandiva/cache.cc
@@ -26,7 +26,7 @@ namespace gandiva {
 static const int DEFAULT_CACHE_SIZE = 5000;
 
 namespace internal {
-int GetCapacityFromEnvVar() {
+int GetCacheCapacityFromEnvVar() {
   auto maybe_env_value = ::arrow::internal::GetEnvVar("GANDIVA_CACHE_SIZE");
   if (!maybe_env_value.ok()) {
     return DEFAULT_CACHE_SIZE;
@@ -52,8 +52,11 @@ int GetCapacityFromEnvVar() {
 }
 }  // namespace internal
 
-int GetCapacity() {
-  static const int capacity = internal::GetCapacityFromEnvVar();
+// Deprecated in 17.0.0. Use GetCacheCapacity instead.
+int GetCapacity() { return GetCacheCapacity(); }
+
+int GetCacheCapacity() {
+  static const int capacity = internal::GetCacheCapacityFromEnvVar();
   return capacity;
 }
 

diff --git a/cpp/src/gandiva/cache.h b/cpp/src/gandiva/cache.h
@@ -20,22 +20,27 @@
 #include <cstdlib>
 #include <mutex>
 
+#include "arrow/util/macros.h"
 #include "gandiva/lru_cache.h"
 #include "gandiva/visibility.h"
 
 namespace gandiva {
 
 namespace internal {
-// Only called once by GetCapacity().
-// Do the actual work of getting the capacity from env var.
+// Only called once by GetCacheCapacity().
+// Do the actual work of getting the cache capacity from env var.
 // Also makes the testing easier.
 GANDIVA_EXPORT
-int GetCapacityFromEnvVar();
+int GetCacheCapacityFromEnvVar();
 }  // namespace internal
 
+ARROW_DEPRECATED("Deprecated in 17.0.0. Use GetCacheCapacity instead.")
 GANDIVA_EXPORT
 int GetCapacity();
 
+GANDIVA_EXPORT
+int GetCacheCapacity();
+
 GANDIVA_EXPORT
 void LogCacheSize(size_t capacity);
 
@@ -44,7 +49,7 @@ class Cache {
  public:
   explicit Cache(size_t capacity) : cache_(capacity) { LogCacheSize(capacity); }
 
-  Cache() : Cache(GetCapacity()) {}
+  Cache() : Cache(GetCacheCapacity()) {}
 
   ValueType GetObjectCode(const KeyType& cache_key) {
     std::optional<ValueType> result;

diff --git a/cpp/src/gandiva/cache_test.cc b/cpp/src/gandiva/cache_test.cc
@@ -43,75 +43,65 @@ TEST(TestCache, TestGetPut) {
 }
 
 namespace {
-constexpr auto capacity_env_var = "GANDIVA_CACHE_SIZE";
-constexpr auto default_capacity = 5000;
+constexpr auto cache_capacity_env_var = "GANDIVA_CACHE_SIZE";
+constexpr auto default_cache_capacity = 5000;
 }  // namespace
 
 TEST(TestCache, TestGetCacheCapacityDefault) {
-  ASSERT_EQ(GetCapacity(), default_capacity);
+  ASSERT_EQ(GetCacheCapacity(), default_cache_capacity);
 }
 
 TEST(TestCache, TestGetCacheCapacityEnvVar) {
-  // Uncleared env var may have side-effect to subsequent tests. Use a structure to help
-  // clearing the env var when leaving the scope.
-  struct ScopedEnvVar {
-    ScopedEnvVar(const char* name, const char* value) : name_(std::move(name)) {
-      ARROW_CHECK_OK(::arrow::internal::SetEnvVar(name_, value));
-    }
-    ~ScopedEnvVar() { ARROW_CHECK_OK(::arrow::internal::DelEnvVar(name_)); }
-
-   private:
-    const char* name_;
-  };
+  using ::arrow::EnvVarGuard;
 
   // Empty.
   {
-    ScopedEnvVar env(capacity_env_var, "");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, "");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 
   // Non-number.
   {
-    ScopedEnvVar env(capacity_env_var, "invalid");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, "invalid");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 
   // Number with invalid suffix.
   {
-    ScopedEnvVar env(capacity_env_var, "42MB");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, "42MB");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 
   // Valid positive number.
   {
-    ScopedEnvVar env(capacity_env_var, "42");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), 42);
+    EnvVarGuard guard(cache_capacity_env_var, "42");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), 42);
   }
 
   // Int max.
   {
     auto str = std::to_string(std::numeric_limits<int>::max());
-    ScopedEnvVar env(capacity_env_var, str.c_str());
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), std::numeric_limits<int>::max());
+    EnvVarGuard guard(cache_capacity_env_var, str.c_str());
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), std::numeric_limits<int>::max());
   }
 
   // Zero.
   {
-    ScopedEnvVar env(capacity_env_var, "0");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, "0");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 
   // Negative number.
   {
-    ScopedEnvVar env(capacity_env_var, "-1");
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, "-1");
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 
   // Over int max.
   {
     auto str = std::to_string(static_cast<int64_t>(std::numeric_limits<int>::max()) + 1);
-    ScopedEnvVar env(capacity_env_var, str.c_str());
-    ASSERT_EQ(internal::GetCapacityFromEnvVar(), default_capacity);
+    EnvVarGuard guard(cache_capacity_env_var, str.c_str());
+    ASSERT_EQ(internal::GetCacheCapacityFromEnvVar(), default_cache_capacity);
   }
 }