Copilot commented on code in PR #12405:
URL: https://github.com/apache/gluten/pull/12405#discussion_r3571968534
##########
cpp/velox/utils/ConfigExtractor.cc:
##########
@@ -294,6 +294,30 @@ std::string getConfigValue(
return got->second;
}
+std::shared_ptr<facebook::velox::config::ConfigBase> mergeWithSessionOverrides(
+ const std::shared_ptr<facebook::velox::config::ConfigBase>& baseConf,
+ const std::unordered_map<std::string, std::string>& sessionConf) {
+ auto merged = baseConf->rawConfigs(); // copy — we must not mutate the
shared base
+
+ // ── Step 1: forward all per-account and generic fs.azure.* / fs.s3a.* /
fs.gs.*
+ // keys from the session config into the merged map. Session value always
wins.
+ static constexpr std::string_view kForwardPrefixes[] = {
+ "fs.azure.",
+ "fs.s3a.",
+ "fs.gs.",
+ };
+ for (const auto& [k, v] : sessionConf) {
+ for (const auto& prefix : kForwardPrefixes) {
+ if (k.starts_with(prefix)) {
+ merged[k] = v;
+ break;
+ }
+ }
+ }
Review Comment:
mergeWithSessionOverrides only forwards keys that already start with
"fs.azure." / "fs.s3a." / "fs.gs.". However, the native session conf coming
from Spark is typically namespaced under "spark.hadoop." (e.g.
"spark.hadoop.fs.azure..." and "spark.hadoop.fs.s3a..."; see
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala
where SPARK_S3_* keys are included in native session conf). As a result,
per-session filesystem overrides won’t be merged into the connector config,
which defeats the goal of delayed connector registration.
Consider normalizing session keys by stripping the optional "spark.hadoop."
prefix before applying the forwarded-prefix check, and write the normalized key
into the merged map (so it matches the keyspace used in
createHiveConnectorConfig).
##########
cpp/velox/tests/ConfigExtractorTest.cc:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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/ConfigExtractor.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <unordered_map>
+
+#include "velox/common/config/Config.h"
+
+namespace gluten {
+
+using ConfigBase = facebook::velox::config::ConfigBase;
+
+// Helper: build a ConfigBase from a braced-init map.
+static std::shared_ptr<ConfigBase> makeBase(std::unordered_map<std::string,
std::string> m) {
+ return std::make_shared<ConfigBase>(std::move(m));
+}
+
+// ── mergeWithSessionOverrides
─────────────────────────────────────────────────
+
+// Keys that match one of the forwarded prefixes must appear in the result.
+TEST(MergeWithSessionOverridesTest, forwardsFsS3aKey) {
+ auto base = makeBase({{"other.key", "base-value"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
Review Comment:
The unit test uses session keys like "fs.s3a.access.key", but Spark/Gluten
session configs are typically passed as "spark.hadoop.fs.s3a.*" (see
docs/get-started/VeloxS3.md and
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala
where SPARK_S3_* keys are included in native session conf). Using the
non-Spark-prefixed form here makes the test less representative and can hide
forwarding bugs.
##########
cpp/velox/tests/ConfigExtractorTest.cc:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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/ConfigExtractor.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <unordered_map>
+
+#include "velox/common/config/Config.h"
+
+namespace gluten {
+
+using ConfigBase = facebook::velox::config::ConfigBase;
+
+// Helper: build a ConfigBase from a braced-init map.
+static std::shared_ptr<ConfigBase> makeBase(std::unordered_map<std::string,
std::string> m) {
+ return std::make_shared<ConfigBase>(std::move(m));
+}
+
+// ── mergeWithSessionOverrides
─────────────────────────────────────────────────
+
+// Keys that match one of the forwarded prefixes must appear in the result.
+TEST(MergeWithSessionOverridesTest, forwardsFsS3aKey) {
+ auto base = makeBase({{"other.key", "base-value"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("fs.s3a.access.key"), "SESSION_ACCESS_KEY");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsAzureKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.azure.account.key.myaccount.dfs.core.windows.net", "TOKEN"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+
EXPECT_EQ(result->rawConfigs().at("fs.azure.account.key.myaccount.dfs.core.windows.net"),
"TOKEN");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsGsKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.gs.auth.service.account.email",
"[email protected]"},
+ };
Review Comment:
This test uses "fs.gs.*" session keys, but ConfigExtractor.cc reads GCS
configs from Spark as "spark.hadoop.fs.gs.*" when building the connector
config. Using the Spark-prefixed form here ensures the test reflects the real
session config key format.
##########
cpp/velox/tests/ConfigExtractorTest.cc:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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/ConfigExtractor.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <unordered_map>
+
+#include "velox/common/config/Config.h"
+
+namespace gluten {
+
+using ConfigBase = facebook::velox::config::ConfigBase;
+
+// Helper: build a ConfigBase from a braced-init map.
+static std::shared_ptr<ConfigBase> makeBase(std::unordered_map<std::string,
std::string> m) {
+ return std::make_shared<ConfigBase>(std::move(m));
+}
+
+// ── mergeWithSessionOverrides
─────────────────────────────────────────────────
+
+// Keys that match one of the forwarded prefixes must appear in the result.
+TEST(MergeWithSessionOverridesTest, forwardsFsS3aKey) {
+ auto base = makeBase({{"other.key", "base-value"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("fs.s3a.access.key"), "SESSION_ACCESS_KEY");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsAzureKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.azure.account.key.myaccount.dfs.core.windows.net", "TOKEN"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+
EXPECT_EQ(result->rawConfigs().at("fs.azure.account.key.myaccount.dfs.core.windows.net"),
"TOKEN");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsGsKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.gs.auth.service.account.email",
"[email protected]"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ EXPECT_EQ(result->rawConfigs().at("fs.gs.auth.service.account.email"),
"[email protected]");
+}
+
+// A session key that does NOT match any forwarded prefix must be ignored.
+TEST(MergeWithSessionOverridesTest, ignoresNonPrefixedKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"spark.some.other.key", "value"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ EXPECT_EQ(result->rawConfigs().count("spark.some.other.key"), 0u);
+}
+
+// Base-config keys that are not overridden must pass through unchanged.
+TEST(MergeWithSessionOverridesTest, preservesUnrelatedBaseKeys) {
+ auto base = makeBase({{"hive.s3.aws-access-key", "BASE_KEY"},
{"hive.s3.aws-secret-key", "BASE_SECRET"}});
+ std::unordered_map<std::string, std::string> session = {};
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("hive.s3.aws-access-key"), "BASE_KEY");
+ EXPECT_EQ(raw.at("hive.s3.aws-secret-key"), "BASE_SECRET");
+}
+
+// Session value must win over the same key already present in the base.
+TEST(MergeWithSessionOverridesTest, sessionValueOverridesBaseValue) {
+ auto base = makeBase({{"fs.s3a.access.key", "BASE_ACCESS_KEY"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
Review Comment:
This override test uses the non-Spark-prefixed key form
("fs.s3a.access.key"). If mergeWithSessionOverrides is intended to consume
Spark session configs from veloxCfg_->rawConfigs(), this should use
"spark.hadoop.fs.s3a.access.key" to match the actual input key format.
##########
cpp/velox/tests/ConfigExtractorTest.cc:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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/ConfigExtractor.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <unordered_map>
+
+#include "velox/common/config/Config.h"
+
+namespace gluten {
+
+using ConfigBase = facebook::velox::config::ConfigBase;
+
+// Helper: build a ConfigBase from a braced-init map.
+static std::shared_ptr<ConfigBase> makeBase(std::unordered_map<std::string,
std::string> m) {
+ return std::make_shared<ConfigBase>(std::move(m));
+}
+
+// ── mergeWithSessionOverrides
─────────────────────────────────────────────────
+
+// Keys that match one of the forwarded prefixes must appear in the result.
+TEST(MergeWithSessionOverridesTest, forwardsFsS3aKey) {
+ auto base = makeBase({{"other.key", "base-value"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("fs.s3a.access.key"), "SESSION_ACCESS_KEY");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsAzureKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.azure.account.key.myaccount.dfs.core.windows.net", "TOKEN"},
+ };
Review Comment:
This test uses "fs.azure.*" keys in the session map, but Spark/Gluten
typically provide ABFS configs as "spark.hadoop.fs.azure.*" (see
docs/get-started/VeloxABFS.md, and ConfigExtractor.cc which strips the
"spark.hadoop." prefix when building ABFS connector config). Using the
Spark-prefixed key here better matches real session input.
##########
cpp/velox/tests/ConfigExtractorTest.cc:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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/ConfigExtractor.h"
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <unordered_map>
+
+#include "velox/common/config/Config.h"
+
+namespace gluten {
+
+using ConfigBase = facebook::velox::config::ConfigBase;
+
+// Helper: build a ConfigBase from a braced-init map.
+static std::shared_ptr<ConfigBase> makeBase(std::unordered_map<std::string,
std::string> m) {
+ return std::make_shared<ConfigBase>(std::move(m));
+}
+
+// ── mergeWithSessionOverrides
─────────────────────────────────────────────────
+
+// Keys that match one of the forwarded prefixes must appear in the result.
+TEST(MergeWithSessionOverridesTest, forwardsFsS3aKey) {
+ auto base = makeBase({{"other.key", "base-value"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("fs.s3a.access.key"), "SESSION_ACCESS_KEY");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsAzureKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.azure.account.key.myaccount.dfs.core.windows.net", "TOKEN"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+
EXPECT_EQ(result->rawConfigs().at("fs.azure.account.key.myaccount.dfs.core.windows.net"),
"TOKEN");
+}
+
+TEST(MergeWithSessionOverridesTest, forwardsFsGsKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.gs.auth.service.account.email",
"[email protected]"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ EXPECT_EQ(result->rawConfigs().at("fs.gs.auth.service.account.email"),
"[email protected]");
+}
+
+// A session key that does NOT match any forwarded prefix must be ignored.
+TEST(MergeWithSessionOverridesTest, ignoresNonPrefixedKey) {
+ auto base = makeBase({});
+ std::unordered_map<std::string, std::string> session = {
+ {"spark.some.other.key", "value"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ EXPECT_EQ(result->rawConfigs().count("spark.some.other.key"), 0u);
+}
+
+// Base-config keys that are not overridden must pass through unchanged.
+TEST(MergeWithSessionOverridesTest, preservesUnrelatedBaseKeys) {
+ auto base = makeBase({{"hive.s3.aws-access-key", "BASE_KEY"},
{"hive.s3.aws-secret-key", "BASE_SECRET"}});
+ std::unordered_map<std::string, std::string> session = {};
+
+ auto result = mergeWithSessionOverrides(base, session);
+ const auto& raw = result->rawConfigs();
+
+ EXPECT_EQ(raw.at("hive.s3.aws-access-key"), "BASE_KEY");
+ EXPECT_EQ(raw.at("hive.s3.aws-secret-key"), "BASE_SECRET");
+}
+
+// Session value must win over the same key already present in the base.
+TEST(MergeWithSessionOverridesTest, sessionValueOverridesBaseValue) {
+ auto base = makeBase({{"fs.s3a.access.key", "BASE_ACCESS_KEY"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION_ACCESS_KEY"},
+ };
+
+ auto result = mergeWithSessionOverrides(base, session);
+ EXPECT_EQ(result->rawConfigs().at("fs.s3a.access.key"),
"SESSION_ACCESS_KEY");
+}
+
+// The original base config must not be mutated.
+TEST(MergeWithSessionOverridesTest, doesNotMutateBase) {
+ auto base = makeBase({{"fs.s3a.access.key", "ORIGINAL"}});
+ std::unordered_map<std::string, std::string> session = {
+ {"fs.s3a.access.key", "SESSION"},
+ };
Review Comment:
Same issue in doesNotMutateBase: if the production input is Spark session
conf, the session override key should use the Spark-prefixed form
("spark.hadoop.fs.s3a.*") rather than "fs.s3a.*" so the test matches real
runtime behavior.
--
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]
