Copilot commented on code in PR #12405:
URL: https://github.com/apache/gluten/pull/12405#discussion_r3553005405
##########
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 const std::vector<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 session keys that already start with
"fs.azure." / "fs.s3a." / "fs.gs.". However, the config extraction path (e.g.
getAbfsHiveConfig) expects Spark to provide session configs as
"spark.hadoop.fs.*" and strips the leading "spark.hadoop." when building the
connector config. As-is, session-level overrides like "spark.hadoop.fs.azure.*"
won’t be forwarded, so the intended ABFS session overrides won’t take effect.
##########
cpp/velox/compute/VeloxRuntime.cc:
##########
@@ -284,23 +283,26 @@ void VeloxRuntime::initializeExecutors() {
void VeloxRuntime::registerConnectors() {
auto* backend = VeloxBackend::get();
+
+ auto merged = mergeWithSessionOverrides(backend->getStaticConnectorConfig(),
veloxCfg_->rawConfigs());
+
Review Comment:
registerConnectors() is no longer called from the VeloxRuntime constructor,
and is now invoked from WholeStageResultIterator’s constructor. If
VeloxRuntime::createResultIterator() is ever called more than once on the same
VeloxRuntime instance, this will attempt to re-register connectors with the
same scoped IDs and likely fail. Make registerConnectors() idempotent for a
given runtime by returning early when connectors are already registered.
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -141,6 +143,11 @@ WholeStageResultIterator::WholeStageResultIterator(
GLUTEN_CHECK(fileSystem != nullptr, "File System for spilling is null!");
fileSystem->mkdir(spillDir);
+ // Prepare for the session level configurations and pass to connectors
+
+ // register the hive connectors
+ veloxRuntime_->registerConnectors();
+
Review Comment:
WholeStageResultIterator dereferences veloxRuntime_ unconditionally. Since
this is a raw pointer passed across translation units, add a defensive check
before using it to avoid a null-dereference if a future call site passes
nullptr.
##########
cpp/velox/compute/VeloxRuntime.h:
##########
@@ -141,9 +141,11 @@ class VeloxRuntime final : public Runtime {
std::vector<facebook::velox::core::PlanNodeId>& scanIds,
std::vector<facebook::velox::core::PlanNodeId>& streamIds);
+ // Need to make this public to be used in WholeStageResultIterator.cc
+ void registerConnectors();
+
Review Comment:
Making VeloxRuntime::registerConnectors() public solely so
WholeStageResultIterator can call it increases coupling and exposes an internal
lifecycle method on the public Runtime API surface. Consider instead ensuring
connectors are registered inside VeloxRuntime::createResultIterator() (or an
internal ensureConnectorsRegistered() helper) before constructing
WholeStageResultIterator, so the iterator doesn’t need a VeloxRuntime* at all.
##########
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
Review Comment:
The new session-override merge logic is easy to regress (especially around
stripping "spark.hadoop." and key prefix forwarding). Please add a small gtest
that builds a base connector config containing e.g. "fs.azure.*" and a session
map containing "spark.hadoop.fs.azure.*", then verifies the merged config
contains the stripped key and that the session value wins.
--
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]
