Refactor to migrate to velox::config::ConfigBase class in the codebase (

facebookincubator#10716) Summary: As part of the consolidating and polishing config effort, all configs should now utilize common/config/Config.h as it provides richer functionality. Deprecate the old one to make the codebase clean Pull Request resolved: facebookincubator#10716 Reviewed By: xiaoxmeng Differential Revision: D61147922 Pulled By: tanjialiang fbshipit-source-id: 751cc47b17d551e882f4172e6ea05c851daa5a81
xiaoxmeng · Aug 14, 2024 · 3ef1d61 · 3ef1d61
1 parent d176894
commit 3ef1d61
Show file tree

Hide file tree

Showing 74 changed files with 509 additions and 498 deletions.
diff --git a/velox/benchmarks/filesystem/ReadBenchmark.cpp b/velox/benchmarks/filesystem/ReadBenchmark.cpp
@@ -16,11 +16,11 @@
 
 #include "velox/benchmarks/filesystem/ReadBenchmark.h"
 
+#include "velox/common/config/Config.h"
 #include "velox/connectors/hive/storage_adapters/abfs/RegisterAbfsFileSystem.h"
 #include "velox/connectors/hive/storage_adapters/gcs/RegisterGCSFileSystem.h"
 #include "velox/connectors/hive/storage_adapters/hdfs/RegisterHdfsFileSystem.h"
 #include "velox/connectors/hive/storage_adapters/s3fs/RegisterS3FileSystem.h"
-#include "velox/core/Config.h"
 
 DEFINE_string(path, "", "Path of the input file");
 DEFINE_int64(
@@ -60,7 +60,7 @@ DEFINE_validator(path, &notEmpty);
 
 namespace facebook::velox {
 
-std::shared_ptr<Config> readConfig(const std::string& filePath) {
+std::shared_ptr<config::ConfigBase> readConfig(const std::string& filePath) {
   std::ifstream configFile(filePath);
   if (!configFile.is_open()) {
     throw std::runtime_error(
@@ -80,7 +80,7 @@ std::shared_ptr<Config> readConfig(const std::string& filePath) {
     properties.emplace(name, value);
   }
 
-  return std::make_shared<facebook::velox::core::MemConfig>(properties);
+  return std::make_shared<config::ConfigBase>(std::move(properties));
 }
 
 // Initialize a LocalReadFile instance for the specified 'path'.
@@ -108,7 +108,7 @@ void ReadBenchmark::initialize() {
     filesystems::registerGCSFileSystem();
     filesystems::registerHdfsFileSystem();
     filesystems::abfs::registerAbfsFileSystem();
-    std::shared_ptr<Config> config;
+    std::shared_ptr<config::ConfigBase> config;
     if (!FLAGS_config.empty()) {
       config = readConfig(FLAGS_config);
     }

diff --git a/velox/benchmarks/tpch/TpchBenchmark.cpp b/velox/benchmarks/tpch/TpchBenchmark.cpp
@@ -265,8 +265,8 @@ class TpchBenchmark {
             std::to_string(FLAGS_max_coalesced_distance_bytes);
     configurationValues[connector::hive::HiveConfig::kPrefetchRowGroups] =
         std::to_string(FLAGS_parquet_prefetch_rowgroups);
-    auto properties =
-        std::make_shared<const core::MemConfig>(configurationValues);
+    auto properties = std::make_shared<const config::ConfigBase>(
+        std::move(configurationValues));
 
     // Create hive connector with config...
     auto hiveConnector =

diff --git a/velox/common/config/Config.h b/velox/common/config/Config.h
@@ -80,6 +80,8 @@ class ConfigBase {
       bool _mutable = false)
       : configs_(std::move(configs)), mutable_(_mutable) {}
 
+  virtual ~ConfigBase() {}
+
   template <typename T>
   ConfigBase& set(const Entry<T>& entry, const T& val) {
     VELOX_CHECK(mutable_, "Cannot set in immutable config");

diff --git a/velox/common/file/FileSystems.cpp b/velox/common/file/FileSystems.cpp
@@ -30,8 +30,9 @@ constexpr std::string_view kFileScheme("file:");
 
 using RegisteredFileSystems = std::vector<std::pair<
     std::function<bool(std::string_view)>,
-    std::function<std::shared_ptr<
-        FileSystem>(std::shared_ptr<const Config>, std::string_view)>>>;
+    std::function<std::shared_ptr<FileSystem>(
+        std::shared_ptr<const config::ConfigBase>,
+        std::string_view)>>>;
 
 RegisteredFileSystems& registeredFileSystems() {
   // Meyers singleton.
@@ -44,14 +45,14 @@ RegisteredFileSystems& registeredFileSystems() {
 void registerFileSystem(
     std::function<bool(std::string_view)> schemeMatcher,
     std::function<std::shared_ptr<FileSystem>(
-        std::shared_ptr<const Config>,
+        std::shared_ptr<const config::ConfigBase>,
         std::string_view)> fileSystemGenerator) {
   registeredFileSystems().emplace_back(schemeMatcher, fileSystemGenerator);
 }
 
 std::shared_ptr<FileSystem> getFileSystem(
     std::string_view filePath,
-    std::shared_ptr<const Config> properties) {
+    std::shared_ptr<const config::ConfigBase> properties) {
   const auto& filesystems = registeredFileSystems();
   for (const auto& p : filesystems) {
     if (p.first(filePath)) {
@@ -78,7 +79,7 @@ folly::once_flag localFSInstantiationFlag;
 // Implement Local FileSystem.
 class LocalFileSystem : public FileSystem {
  public:
-  explicit LocalFileSystem(std::shared_ptr<const Config> config)
+  explicit LocalFileSystem(std::shared_ptr<const config::ConfigBase> config)
       : FileSystem(config) {}
 
   ~LocalFileSystem() override {}
@@ -193,9 +194,9 @@ class LocalFileSystem : public FileSystem {
   }
 
   static std::function<std::shared_ptr<
-      FileSystem>(std::shared_ptr<const Config>, std::string_view)>
+      FileSystem>(std::shared_ptr<const config::ConfigBase>, std::string_view)>
   fileSystemGenerator() {
-    return [](std::shared_ptr<const Config> properties,
+    return [](std::shared_ptr<const config::ConfigBase> properties,
               std::string_view filePath) {
       // One instance of Local FileSystem is sufficient.
       // Initialize on first access and reuse after that.

diff --git a/velox/common/file/FileSystems.h b/velox/common/file/FileSystems.h
@@ -23,7 +23,9 @@
 #include <string_view>
 
 namespace facebook::velox {
-class Config;
+namespace config {
+class ConfigBase;
+}
 class ReadFile;
 class WriteFile;
 } // namespace facebook::velox
@@ -50,7 +52,7 @@ struct FileOptions {
 /// An abstract FileSystem
 class FileSystem {
  public:
-  FileSystem(std::shared_ptr<const Config> config)
+  FileSystem(std::shared_ptr<const config::ConfigBase> config)
       : config_(std::move(config)) {}
   virtual ~FileSystem() = default;
 
@@ -101,12 +103,12 @@ class FileSystem {
   virtual void rmdir(std::string_view path) = 0;
 
  protected:
-  std::shared_ptr<const Config> config_;
+  std::shared_ptr<const config::ConfigBase> config_;
 };
 
 std::shared_ptr<FileSystem> getFileSystem(
     std::string_view filename,
-    std::shared_ptr<const Config> config);
+    std::shared_ptr<const config::ConfigBase> config);
 
 /// Returns true if filePath is supported by any registered file system,
 /// otherwise false.
@@ -120,7 +122,7 @@ bool isPathSupportedByRegisteredFileSystems(const std::string_view& filePath);
 void registerFileSystem(
     std::function<bool(std::string_view)> schemeMatcher,
     std::function<std::shared_ptr<FileSystem>(
-        std::shared_ptr<const Config>,
+        std::shared_ptr<const config::ConfigBase>,
         std::string_view)> fileSystemGenerator);
 
 /// Register the local filesystem.

diff --git a/velox/common/file/tests/FaultyFileSystem.cpp b/velox/common/file/tests/FaultyFileSystem.cpp
@@ -38,9 +38,9 @@ std::function<bool(std::string_view)> schemeMatcher() {
 folly::once_flag faultFilesystemInitOnceFlag;
 
 std::function<std::shared_ptr<
-    FileSystem>(std::shared_ptr<const Config>, std::string_view)>
+    FileSystem>(std::shared_ptr<const config::ConfigBase>, std::string_view)>
 fileSystemGenerator() {
-  return [](std::shared_ptr<const Config> properties,
+  return [](std::shared_ptr<const config::ConfigBase> properties,
             std::string_view /*unused*/) {
     // One instance of faulty FileSystem is sufficient. Initializes on first
     // access and reuse after that.

diff --git a/velox/common/file/tests/FaultyFileSystem.h b/velox/common/file/tests/FaultyFileSystem.h
@@ -32,7 +32,7 @@ using namespace filesystems;
 /// file operation to the real file system underneath.
 class FaultyFileSystem : public FileSystem {
  public:
-  explicit FaultyFileSystem(std::shared_ptr<const Config> config)
+  explicit FaultyFileSystem(std::shared_ptr<const config::ConfigBase> config)
       : FileSystem(std::move(config)) {}
 
   ~FaultyFileSystem() override {}

diff --git a/velox/connectors/CMakeLists.txt b/velox/connectors/CMakeLists.txt
@@ -13,7 +13,7 @@
 # limitations under the License.
 velox_add_library(velox_connector Connector.cpp)
 
-velox_link_libraries(velox_connector velox_config velox_vector)
+velox_link_libraries(velox_connector velox_common_config velox_vector)
 
 add_subdirectory(fuzzer)
 

diff --git a/velox/connectors/Connector.h b/velox/connectors/Connector.h
@@ -29,15 +29,17 @@
 
 #include <folly/Synchronized.h>
 
+namespace facebook::velox {
+class Config;
+}
 namespace facebook::velox::wave {
 class WaveDataSource;
 }
 namespace facebook::velox::common {
 class Filter;
 }
-
-namespace facebook::velox {
-class Config;
+namespace facebook::velox::config {
+class ConfigBase;
 }
 
 namespace facebook::velox::connector {
@@ -256,7 +258,7 @@ class ConnectorQueryCtx {
   ConnectorQueryCtx(
       memory::MemoryPool* operatorPool,
       memory::MemoryPool* connectorPool,
-      const Config* sessionProperties,
+      const config::ConfigBase* sessionProperties,
       const common::SpillConfig* spillConfig,
       common::PrefixSortConfig prefixSortConfig,
       std::unique_ptr<core::ExpressionEvaluator> expressionEvaluator,
@@ -297,7 +299,7 @@ class ConnectorQueryCtx {
     return connectorPool_;
   }
 
-  const Config* sessionProperties() const {
+  const config::ConfigBase* sessionProperties() const {
     return sessionProperties_;
   }
 
@@ -356,7 +358,7 @@ class ConnectorQueryCtx {
  private:
   memory::MemoryPool* const operatorPool_;
   memory::MemoryPool* const connectorPool_;
-  const Config* const sessionProperties_;
+  const config::ConfigBase* const sessionProperties_;
   const common::SpillConfig* const spillConfig_;
   const common::PrefixSortConfig prefixSortConfig_;
   std::unique_ptr<core::ExpressionEvaluator> expressionEvaluator_;
@@ -380,7 +382,8 @@ class Connector {
     return id_;
   }
 
-  virtual const std::shared_ptr<const Config>& connectorConfig() const {
+  virtual const std::shared_ptr<const config::ConfigBase>& connectorConfig()
+      const {
     VELOX_NYI("connectorConfig is not supported yet");
   }
 
@@ -447,6 +450,12 @@ class ConnectorFactory {
     return name_;
   }
 
+  virtual std::shared_ptr<Connector> newConnector(
+      const std::string& id,
+      std::shared_ptr<const config::ConfigBase> config,
+      folly::Executor* executor = nullptr) = 0;
+
+  // TODO(jtan6): [Config Refactor] Remove this old API when refactor is done.
   virtual std::shared_ptr<Connector> newConnector(
       const std::string& id,
       std::shared_ptr<const Config> config,

diff --git a/velox/connectors/fuzzer/FuzzerConnector.h b/velox/connectors/fuzzer/FuzzerConnector.h
@@ -15,8 +15,10 @@
  */
 #pragma once
 
+#include "velox/common/config/Config.h"
 #include "velox/connectors/Connector.h"
 #include "velox/connectors/fuzzer/FuzzerConnectorSplit.h"
+#include "velox/core/Config.h"
 #include "velox/vector/fuzzer/VectorFuzzer.h"
 
 namespace facebook::velox::connector::fuzzer {
@@ -103,7 +105,7 @@ class FuzzerConnector final : public Connector {
  public:
   FuzzerConnector(
       const std::string& id,
-      std::shared_ptr<const Config> config,
+      std::shared_ptr<const config::ConfigBase> config,
       folly::Executor* /*executor*/)
       : Connector(id) {}
 
@@ -139,10 +141,21 @@ class FuzzerConnectorFactory : public ConnectorFactory {
 
   std::shared_ptr<Connector> newConnector(
       const std::string& id,
-      std::shared_ptr<const Config> config,
+      std::shared_ptr<const config::ConfigBase> config,
       folly::Executor* executor = nullptr) override {
     return std::make_shared<FuzzerConnector>(id, config, executor);
   }
+
+  std::shared_ptr<Connector> newConnector(
+      const std::string& id,
+      std::shared_ptr<const Config> config,
+      folly::Executor* executor = nullptr) override {
+    std::shared_ptr<const config::ConfigBase> convertedConfig;
+    convertedConfig = config == nullptr
+        ? nullptr
+        : std::make_shared<config::ConfigBase>(config->valuesCopy());
+    return newConnector(id, convertedConfig, executor);
+  }
 };
 
 } // namespace facebook::velox::connector::fuzzer
diff --git a/velox/connectors/fuzzer/tests/FuzzerConnectorTestBase.h b/velox/connectors/fuzzer/tests/FuzzerConnectorTestBase.h
@@ -26,10 +26,11 @@ class FuzzerConnectorTestBase : public exec::test::OperatorTestBase {
 
   void SetUp() override {
     OperatorTestBase::SetUp();
+    std::shared_ptr<const config::ConfigBase> config;
     auto fuzzerConnector =
         connector::getConnectorFactory(
             connector::fuzzer::FuzzerConnectorFactory::kFuzzerConnectorName)
-            ->newConnector(kFuzzerConnectorId, nullptr);
+            ->newConnector(kFuzzerConnectorId, config);
     connector::registerConnector(fuzzerConnector);
   }
 

diff --git a/velox/connectors/hive/FileHandle.h b/velox/connectors/hive/FileHandle.h
@@ -27,13 +27,12 @@
 
 #include "velox/common/caching/CachedFactory.h"
 #include "velox/common/caching/FileIds.h"
+#include "velox/common/config/Config.h"
 #include "velox/common/file/File.h"
 #include "velox/connectors/hive/FileProperties.h"
 
 namespace facebook::velox {
 
-class Config;
-
 // See the file comment.
 struct FileHandle {
   std::shared_ptr<ReadFile> file;
@@ -66,14 +65,14 @@ using FileHandleCache = SimpleLRUCache<std::string, FileHandle>;
 class FileHandleGenerator {
  public:
   FileHandleGenerator() {}
-  FileHandleGenerator(std::shared_ptr<const Config> properties)
+  FileHandleGenerator(std::shared_ptr<const config::ConfigBase> properties)
       : properties_(std::move(properties)) {}
   std::unique_ptr<FileHandle> operator()(
       const std::string& filename,
       const FileProperties* properties);
 
  private:
-  const std::shared_ptr<const Config> properties_;
+  const std::shared_ptr<const config::ConfigBase> properties_;
 };
 
 using FileHandleFactory = CachedFactory<