[native] Translate array agg config to velox

presto-native-execution/presto_cpp/main/common/Configs.cpp

@@ -192,6 +192,7 @@ SystemConfig::SystemConfig() {
           STR_PROP(kInternalCommunicationJwtEnabled, "false"),
           STR_PROP(kInternalCommunicationSharedSecret, ""),
           NUM_PROP(kInternalCommunicationJwtExpirationSeconds, 300),
+          BOOL_PROP(kUseLegacyArrayAgg, false),
       };
 }
 
@@ -497,6 +498,10 @@ int32_t SystemConfig::internalCommunicationJwtExpirationSeconds() const {
       .value();
 }
 
+bool SystemConfig::useLegacyArrayAgg() const {
+  return optionalProperty<bool>(kUseLegacyArrayAgg).value();
+}
+
 NodeConfig::NodeConfig() {
   registeredProps_ =
       std::unordered_map<std::string, folly::Optional<std::string>>{
@@ -650,8 +655,8 @@ BaseVeloxQueryConfig::BaseVeloxQueryConfig() {
               QueryConfig::kSpillableReservationGrowthPct,
               c.spillableReservationGrowthPct()),
           BOOL_PROP(
-              QueryConfig::kSparkLegacySizeOfNull, c.sparkLegacySizeOfNull()),
-      };
+              QueryConfig::kSparkLegacySizeOfNull, c.sparkLegacySizeOfNull())};
+  update(*SystemConfig::instance());
 }
 
 BaseVeloxQueryConfig* BaseVeloxQueryConfig::instance() {
@@ -660,4 +665,14 @@ BaseVeloxQueryConfig* BaseVeloxQueryConfig::instance() {
   return instance.get();
 }
 
+void BaseVeloxQueryConfig::initialize(const std::string& filePath) {
+  ConfigBase::initialize(filePath);
+  update(*SystemConfig::instance());
+}
+
+void BaseVeloxQueryConfig::update(const SystemConfig& systemConfig) {
+  registeredProps_[velox::core::QueryConfig::kPrestoArrayAggIgnoreNulls] =
+      bool2String(systemConfig.useLegacyArrayAgg());
+}
+
 } // namespace facebook::presto

presto-native-execution/presto_cpp/main/common/Configs.h

@@ -29,7 +29,7 @@ class ConfigBase {
   /// Reads configuration properties from the specified file. Must be called
   /// before calling any of the getters below.
   /// @param filePath Path to configuration file.
-  void initialize(const std::string& filePath);
+  virtual void initialize(const std::string& filePath);
 
   /// Uses a config object already materialized.
   void initialize(std::unique_ptr<velox::Config>&& config) {
@@ -130,6 +130,8 @@ class ConfigBase {
     return config_->valuesCopy();
   }
 
+  virtual ~ConfigBase() = default;
+
  protected:
   ConfigBase();
 
@@ -339,8 +341,14 @@ class SystemConfig : public ConfigBase {
   static constexpr std::string_view kInternalCommunicationJwtExpirationSeconds{
       "internal-communication.jwt.expiration-seconds"};
 
+  /// Uses legacy version of array_agg which ignores nulls.
+  static constexpr std::string_view kUseLegacyArrayAgg{
+      "deprecated.legacy-array-agg"};
+
   SystemConfig();
 
+  virtual ~SystemConfig() = default;
+
   static SystemConfig* instance();
 
   int httpServerHttpPort() const;
@@ -488,6 +496,8 @@ class SystemConfig : public ConfigBase {
   std::string internalCommunicationSharedSecret() const;
 
   int32_t internalCommunicationJwtExpirationSeconds() const;
+
+  bool useLegacyArrayAgg() const;
 };
 
 /// Provides access to node properties defined in node.properties file.
@@ -504,6 +514,8 @@ class NodeConfig : public ConfigBase {
 
   NodeConfig();
 
+  virtual ~NodeConfig() = default;
+
   static NodeConfig* instance();
 
   std::string nodeEnvironment() const;
@@ -526,7 +538,15 @@ class BaseVeloxQueryConfig : public ConfigBase {
  public:
   BaseVeloxQueryConfig();
 
+  virtual ~BaseVeloxQueryConfig() = default;
+
+  void initialize(const std::string& filePath) override;
+
   static BaseVeloxQueryConfig* instance();
+
+ private:
+  /// Update velox config with values from presto system config.
+  void update(const SystemConfig& config);
 };
 
 } // namespace facebook::presto

presto-native-execution/presto_cpp/main/common/tests/BaseVeloxQueryConfigTest.cpp

@@ -25,7 +25,7 @@ using namespace velox::core;
 
 class BaseVeloxQueryConfigTest : public testing::Test {
  protected:
-  void setUpConfigFile(bool isMutable) {
+  void setUpConfigFile(bool isMutable, bool setupSystemConfig = false) {
     velox::filesystems::registerLocalFileSystem();
 
     char path[] = "/tmp/base_velox_query_config_test_XXXXXX";
@@ -35,6 +35,19 @@ class BaseVeloxQueryConfigTest : public testing::Test {
     }
     configFilePath = tempDirectoryPath;
     configFilePath += "/velox.properties";
+    systemConfigFilePath = tempDirectoryPath;
+    systemConfigFilePath += "/config.properties";
+
+    if (setupSystemConfig) {
+      auto fileSystem =
+          filesystems::getFileSystem(systemConfigFilePath, nullptr);
+      auto systemConfigFile =
+          fileSystem->openFileForWrite(systemConfigFilePath);
+      systemConfigFile->append(
+          fmt::format("{}=true\n", SystemConfig::kUseLegacyArrayAgg));
+      systemConfigFile->close();
+      SystemConfig::instance()->initialize(systemConfigFilePath);
+    }
 
     auto fileSystem = filesystems::getFileSystem(configFilePath, nullptr);
     auto sysConfigFile = fileSystem->openFileForWrite(configFilePath);
@@ -50,6 +63,7 @@ class BaseVeloxQueryConfigTest : public testing::Test {
   }
 
   std::string configFilePath;
+  std::string systemConfigFilePath;
   const std::string tzPropName{QueryConfig::kSessionTimezone};
 };
 
@@ -93,4 +107,16 @@ TEST_F(BaseVeloxQueryConfigTest, mutableConfig) {
       ret = cfg->optionalProperty(tzPropName));
 }
 
+TEST_F(BaseVeloxQueryConfigTest, fromSystemConfig) {
+  auto cfg = BaseVeloxQueryConfig::instance();
+  ASSERT_FALSE(cfg->optionalProperty<bool>(
+                      std::string(QueryConfig::kPrestoArrayAggIgnoreNulls))
+                   .value());
+  setUpConfigFile(true, true);
+  cfg->initialize(configFilePath);
+  ASSERT_TRUE(cfg->optionalProperty<bool>(
+                     std::string(QueryConfig::kPrestoArrayAggIgnoreNulls))
+                  .value());
+}
+
 } // namespace facebook::presto::test