Copilot commented on code in PR #12884: URL: https://github.com/apache/gluten/pull/12884#discussion_r3844796322
########## ep/build-velox/src/get-velox.sh: ########## @@ -17,8 +17,8 @@ set -exu CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd) -VELOX_REPO=https://github.com/IBM/velox.git -VELOX_BRANCH=dft-2026_08_21 +VELOX_REPO=https://github.com/rui-mo/velox-dev.git +VELOX_BRANCH=test VELOX_ENHANCED_BRANCH=ibm-2026_08_21 Review Comment: Switching the build script to a personal fork and a generic `test` branch makes builds non-reproducible and likely breaks CI/release workflows. Please revert to the project-approved Velox repo/branch (or make this overrideable via environment variables while keeping a stable default). ########## gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java: ########## @@ -55,10 +57,28 @@ public enum ReadFileFormat { UnknownFormat() } + public enum ColumnMappingMode { + POSITION("POSITION"), + NAME("NAME"), + PARQUET_FIELD_ID("PARQUET_FIELD_ID"), + FIELD_ID("FIELD_ID"); Review Comment: Having both `PARQUET_FIELD_ID` and `FIELD_ID` in the same public enum is ambiguous (it’s unclear when each should be used and how they differ). Please either remove the redundant value, rename to clearly reflect distinct semantics, or add documentation explaining the difference and ensuring the `nativeName` strings are exactly what the Velox parser expects. ########## 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) Review Comment: This always attaches an explicit mapping mode for ORC/Parquet, including default `NAME`. If the native side treats an explicitly-provided mode as strict, this could unintentionally change prior behavior that relied on native per-file heuristics (e.g., special-casing `_col*` ORC schemas). Consider only setting the metadata when deviating from the native default (e.g., only send `POSITION` when forcing positional), or introduce an explicit `AUTO`/unspecified path to preserve native defaults. ########## cpp/velox/compute/VeloxPlanConverter.cc: ########## @@ -53,7 +53,11 @@ VeloxPlanConverter::VeloxPlanConverter( } namespace { Review Comment: The metadata key string is duplicated across C++ (here) and JVM (`LocalFilesNode.COLUMN_MAPPING_MODE_METADATA_KEY`). To avoid drift, consider centralizing it (e.g., in a shared header/interface, generated constant, or at least referencing the JVM constant in a comment and adding a small unit/integration assertion that the key matches). ########## 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 => + Some( + if (VeloxConfig.get.parquetUseColumnNames) ColumnMappingMode.NAME + else ColumnMappingMode.POSITION) + case _ => + None + } + } Review Comment: This always attaches an explicit mapping mode for ORC/Parquet, including default `NAME`. If the native side treats an explicitly-provided mode as strict, this could unintentionally change prior behavior that relied on native per-file heuristics (e.g., special-casing `_col*` ORC schemas). Consider only setting the metadata when deviating from the native default (e.g., only send `POSITION` when forcing positional), or introduce an explicit `AUTO`/unspecified path to preserve native defaults. -- 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]
