Copilot commented on code in PR #12405:
URL: https://github.com/apache/gluten/pull/12405#discussion_r3559603189
##########
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.",
+ };
Review Comment:
Using std::vector here introduces a dependency on <vector>, but this file
doesn’t include it directly. To keep the translation unit self-contained (and
avoid relying on transitive includes), prefer a simple constexpr array for the
small fixed prefix list.
##########
cpp/velox/compute/WholeStageResultIterator.h:
##########
@@ -131,6 +133,7 @@ class WholeStageResultIterator : public
SplitAwareColumnarBatchIterator {
/// Memory.
VeloxMemoryManager* memoryManager_;
+ VeloxRuntime* veloxRuntime_;
Review Comment:
WholeStageResultIterator stores VeloxRuntime* as a member, but it’s only
used in the constructor to call registerConnectors(). Keeping a raw pointer
member unnecessarily increases lifetime coupling and object size; consider
calling through the ctor parameter (after a null check) and removing the member
entirely unless it’s needed later.
##########
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:
veloxRuntime_ is dereferenced without a null check. Since this pointer is
now an explicit constructor parameter, it’s safer to assert it before use to
avoid a hard-to-debug crash if a caller ever passes nullptr. (Also, the comment
says “hive connectors” but registerConnectors() registers multiple connectors.)
--
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]
