Copilot commented on code in PR #12884:
URL: https://github.com/apache/gluten/pull/12884#discussion_r3894859393
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxIteratorApi.scala:
##########
@@ -66,10 +68,27 @@ class VeloxIteratorApi extends IteratorApi with Logging {
) {
localFilesNode.setFileSchema(fileSchema)
}
+ columnMappingMode(fileFormat).foreach(localFilesNode.setColumnMappingMode)
localFilesNode
}
+ private def columnMappingMode(fileFormat: ReadFileFormat):
Option[ColumnMappingMode] = {
+ fileFormat match {
+ case ReadFileFormat.OrcReadFormat | ReadFileFormat.DwrfReadFormat =>
+ val forcePosition =
+
SQLConf.get.getConfString(GlutenConfig.SPARK_ORC_FORCE_POSITIONAL_EVOLUTION,
"false")
+ .toBoolean
+ Some(if (forcePosition) ColumnMappingMode.POSITION else
ColumnMappingMode.NAME)
+ case ReadFileFormat.ParquetReadFormat =>
Review Comment:
`orc.force.positional.evolution` is a Hadoop/SparkConf-style key
(`spark.hadoop.*`). Reading it solely via `SQLConf.get.getConfString` can miss
values provided via SparkConf / spark-submit, causing ORC/DWRF scans to use
NAME mapping when POSITION was configured (potentially returning wrong columns
or triggering fallback). Prefer falling back to `SparkEnv.get.conf` when the
key is not present in `SQLConf`.
##########
cpp/velox/compute/VeloxPlanConverter.cc:
##########
@@ -164,6 +177,15 @@ std::shared_ptr<SplitInfo> parseScanSplitInfo(
metadataColumnMap[metadataColumn.key()] = metadataColumn.value();
}
for (const auto& otherMetadataColumn :
file.other_const_metadata_columns()) {
+ if (otherMetadataColumn.key() ==
std::string(kColumnMappingModeMetadataKey)) {
+ auto mode = parseColumnMappingMode(otherMetadataColumn.value());
+ VELOX_CHECK(mode.has_value(), "Invalid column mapping mode metadata
for key {}", kColumnMappingModeMetadataKey);
+ if (splitInfo->columnMappingMode.has_value()) {
Review Comment:
In the per-file metadata loop, comparing against
`std::string(kColumnMappingModeMetadataKey)` constructs a new `std::string` for
every metadata entry. Since this code runs once per split and the project uses
C++20, compare the `std::string` key directly with the `std::string_view`
constant to avoid repeated allocations.
--
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]
