Copilot commented on code in PR #12495:
URL: https://github.com/apache/gluten/pull/12495#discussion_r3557792117
##########
cpp/core/config/GlutenConfig.cc:
##########
@@ -33,9 +31,13 @@ std::optional<boost::regex> getRedactionRegex(const
std::unordered_map<std::stri
}
return std::nullopt;
}
Review Comment:
Constructing `boost::regex` can throw `boost::regex_error` for an invalid
user-provided `spark.redaction.regex`. Since this helper is now used by config
dumping paths, a bad regex could crash/abort dumping (or worse, error out in a
production debugging workflow). Consider catching `boost::regex_error` inside
`getRedactionRegex` and returning `std::nullopt` (optionally logging a warning)
so redaction gracefully degrades instead of failing.
##########
backends-velox/src/main/scala/org/apache/gluten/datasource/VeloxDataSourceUtil.scala:
##########
@@ -31,25 +31,60 @@ import java.util
object VeloxDataSourceUtil {
def readSchema(files: Seq[FileStatus]): Option[StructType] = {
+ readSchema(files, new util.HashMap[String, String]())
+ }
+
+ def readSchema(
+ files: Seq[FileStatus],
+ fsConf: util.Map[String, String]): Option[StructType] = {
if (files.isEmpty) {
throw new IllegalArgumentException("No input file specified")
}
- readSchema(files.toList.head)
+ readSchema(files.toList.head, fsConf)
}
def readSchema(file: FileStatus): Option[StructType] = {
+ readSchema(file, new util.HashMap[String, String]())
+ }
+
+ def readSchema(
+ file: FileStatus,
+ fsConf: util.Map[String, String]): Option[StructType] = {
val allocator = ArrowBufferAllocators.contextInstance()
- val runtime = Runtimes.contextInstance(BackendsApiManager.getBackendName,
"VeloxWriter")
+ val runtime =
+ Runtimes.contextInstance(BackendsApiManager.getBackendName,
"VeloxWriter", fsConf)
val datasourceJniWrapper = VeloxDataSourceJniWrapper.create(runtime)
- val dsHandle =
- datasourceJniWrapper.init(file.getPath.toString, -1, new
util.HashMap[String, String]())
- val cSchema = ArrowSchema.allocateNew(allocator)
- datasourceJniWrapper.inspectSchema(dsHandle, cSchema.memoryAddress())
+ withDataSourceResource[ArrowSchema, Option[StructType]](
+ () => datasourceJniWrapper.init(file.getPath.toString, -1, fsConf),
+ () => ArrowSchema.allocateNew(allocator),
+ (dsHandle, cSchema) => {
+ datasourceJniWrapper.inspectSchema(dsHandle, cSchema.memoryAddress())
+
Option(SparkSchemaUtil.fromArrowSchema(ArrowAbiUtil.importToSchema(allocator,
cSchema)))
+ },
+ datasourceJniWrapper.close
+ )
+ }
+
+ private[apache] def withDataSourceResource[R <: AutoCloseable, T](
+ initialize: () => Long,
+ allocate: () => R,
+ use: (Long, R) => T,
+ closeHandle: Long => Unit): T = {
Review Comment:
`private[apache]` makes this helper visible to the entire `org.apache.*`
namespace (including Spark code), which is a very broad exposure for a
low-level resource-management utility. If the intent is test-only access,
prefer narrowing visibility (e.g., `private[gluten]` / `private[datasource]`)
and adjusting the tests’ package (or using a small test-only wrapper) to avoid
effectively making this a quasi-public API.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxIteratorApi.scala:
##########
@@ -303,6 +354,21 @@ class VeloxIteratorApi extends IteratorApi with Logging {
}
object VeloxIteratorApi {
+ private[gluten] def buildExtraConf(
+ fsConf: Map[String, String],
+ enableCudf: Boolean,
+ supportsValueStreamDynamicFilter: Boolean): Map[String, String] = {
+ val dynamicFilterConf =
+ if (supportsValueStreamDynamicFilter) {
+ Map.empty[String, String]
+ } else {
+ Map(VeloxConfig.VALUE_STREAM_DYNAMIC_FILTER_ENABLED.key -> "false")
+ }
+ val controlConf =
+ Map(GlutenConfig.COLUMNAR_CUDF_ENABLED.key -> enableCudf.toString) ++
dynamicFilterConf
+ fsConf ++ controlConf
+ }
Review Comment:
`buildExtraConf` currently forwards *all* keys from `fsConf` into the native
extraConf map. If a caller accidentally passes a broader map (e.g., session
conf containing tokens/UGI identity or other sensitive/non-filesystem keys),
those could be propagated into the native side unexpectedly. Consider
defensively filtering `fsConf` to only `spark.hadoop.fs.*` entries inside this
helper (then overlay `controlConf`) so the contract is enforced at the boundary.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxBackend.scala:
##########
@@ -72,6 +72,7 @@ class VeloxBackend extends SubstraitBackend {
override def settings(): BackendSettingsApi = VeloxBackendSettings
override def convFuncOverride(): ConventionFunc.Override = new ConvFunc()
override def costers(): Seq[LongCoster] = Seq(LegacyCoster, RoughCoster)
+ override def interestedPrefixes(): Set[String] = Set("fs.s3a.", "fs.azure.",
"fs.gs.", "fs.")
Review Comment:
Including both the scheme-specific prefixes and the catch-all `\"fs.\"` is
redundant (the `\"fs.\"` entry already matches all of the others). Dropping the
redundant scheme-specific entries (or dropping the catch-all and keeping
explicit schemes) would make intent clearer and avoid confusion about which are
actually needed.
##########
cpp/velox/utils/ConfigExtractor.cc:
##########
@@ -246,6 +283,38 @@ std::string parquetSessionProperty(std::string_view key) {
} // namespace
+std::shared_ptr<facebook::velox::config::ConfigBase> mergeFileSystemConfigs(
+ const std::shared_ptr<facebook::velox::config::ConfigBase>& backendConf,
+ const std::shared_ptr<facebook::velox::config::ConfigBase>& runtimeConf) {
+ constexpr std::string_view kSparkHadoopFsPrefix = "spark.hadoop.fs.";
+
+ auto merged = backendConf->rawConfigs();
+ for (const auto& [key, value] : runtimeConf->rawConfigs()) {
+ if (key.starts_with(kSparkHadoopFsPrefix)) {
+ merged.insert_or_assign(key, value);
+ }
+ }
Review Comment:
`std::string::starts_with` requires C++20; if this codebase (or some build
variants) compile with C++17, this will fail to build. To keep compatibility,
replace `starts_with(...)` with an explicit prefix check (e.g., `key.size() >=
prefix.size() && key.compare(0, prefix.size(), prefix) == 0`) or use a
project-standard helper for prefix matching.
--
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]
