Copilot commented on code in PR #12405:
URL: https://github.com/apache/gluten/pull/12405#discussion_r3559889716
##########
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;
+ }
+ }
+ }
+
+ return
std::make_shared<facebook::velox::config::ConfigBase>(std::move(merged));
+}
Review Comment:
mergeWithSessionOverrides is currently looking for session keys that already
start with "fs.azure." / "fs.s3a." / "fs.gs.". However sessionConf comes
straight from Spark via parseConfMap (no key normalization), and the existing
extractor logic expects Spark-style keys like "spark.hadoop.fs.azure.*" (see
getAbfsHiveConfig). As a result, session-level overrides (e.g.
spark.hadoop.fs.azure.account.key...) won't be merged into the connector
config, so delayed registration still won't pick them up.
##########
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) {
Review Comment:
New session-level connector override behavior (mergeWithSessionOverrides)
isn’t covered by a unit test. Since ConfigExtractor.cc already has test
coverage (e.g. GlutenS3FileSystemTest validates createHiveConnectorConfig
extraction), add a small test that verifies Spark-style session keys like
"spark.hadoop.fs.azure.*" are forwarded/stripped into "fs.azure.*" in the
merged connector config.
--
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]
