[GLUTEN-6887][VL] Pass Velox column mapping mode through scan splits - #12884
Open
rui-mo wants to merge 1 commit into
Open
[GLUTEN-6887][VL] Pass Velox column mapping mode through scan splits#12884rui-mo wants to merge 1 commit into
rui-mo wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR changes how Velox column mapping mode is propagated: instead of being passed via Velox session configs, it is resolved per scan split and sent through LocalFiles metadata so ORC and Parquet scans in the same query can use different mapping modes without fallback.
Changes:
- Add split-level column mapping mode metadata to Substrait LocalFiles and parse it on the Velox side into
SplitInfo. - Stop forwarding Parquet/ORC column-mapping-related session configs into Velox connector session configs.
- Add a Spark35 UT covering mixed ORC (positional) and Parquet (name) mapping in a single Velox query.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-ut/spark35/src/test/scala/org/apache/spark/sql/hive/execution/GlutenHiveSQLQuerySuite.scala | Adds coverage to ensure ORC positional + Parquet name mapping work together in one Velox query. |
| gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala | Stops passing mapping-mode configs via native session configs for Velox. |
| gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java | Adds column mapping mode metadata on the JVM side for each scan split. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxIteratorApi.scala | Resolves mapping mode per file format and sets it into LocalFilesNode. |
| cpp/velox/compute/VeloxPlanConverter.cc | Parses the new metadata key and stores the mode in SplitInfo. |
| cpp/velox/substrait/SubstraitToVeloxPlan.h | Extends SplitInfo to carry optional column mapping mode. |
| cpp/velox/compute/WholeStageResultIterator.cc | Propagates SplitInfo column mapping mode into Velox connector splits. |
| cpp/velox/utils/ConfigExtractor.cc / cpp/velox/config/VeloxConfig.h | Removes session-config-based column mapping plumbing. |
| ep/build-velox/src/get-velox.sh | Changes Velox repo/branch used for builds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| VELOX_USER_CHECK_NOT_NULL(veloxCfg_); | ||
| } | ||
|
|
||
| namespace { |
Comment on lines
+60
to
+64
| public enum ColumnMappingMode { | ||
| POSITION("POSITION"), | ||
| NAME("NAME"), | ||
| PARQUET_FIELD_ID("PARQUET_FIELD_ID"), | ||
| FIELD_ID("FIELD_ID"); |
| ) { | ||
| localFilesNode.setFileSchema(fileSchema) | ||
| } | ||
| columnMappingMode(fileFormat).foreach(localFilesNode.setColumnMappingMode) |
Comment on lines
+76
to
+90
| 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 | ||
| } | ||
| } |
|
Run Gluten Clickhouse CI on x86 |
| metadataColumnMap[metadataColumn.key()] = metadataColumn.value(); | ||
| } | ||
| for (const auto& otherMetadataColumn : file.other_const_metadata_columns()) { | ||
| if (otherMetadataColumn.key() == kColumnMappingModeMetadataKey) { |
Comment on lines
+20
to
+21
| VELOX_REPO=https://github.com/rui-mo/velox-dev.git | ||
| VELOX_BRANCH=test |
Comment on lines
+156
to
+159
| withSQLConf( | ||
| "spark.sql.hive.convertMetastoreOrc" -> "false", | ||
| "spark.sql.hive.convertMetastoreParquet" -> "false", | ||
| "spark.hadoop.orc.force.positional.evolution" -> "true") { |
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class LocalFilesNode implements SplitInfo { |
|
Run Gluten Clickhouse CI on x86 |
rui-mo
force-pushed
the
wip_map_mode
branch
2 times, most recently
from
August 24, 2026 17:04
b25c9d8 to
26c654e
Compare
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ep/build-velox/src/get-velox.sh:21
- Default Velox source is changed to a personal fork/branch (rui-mo/velox-dev.git:test). This is a supply-chain / reproducibility risk and can break CI or downstream builds. The script already supports --velox_repo/--velox_branch for developer testing Velox PRs; defaults should remain a stable, shared upstream/fork used by the project.
VELOX_REPO=https://github.com/rui-mo/velox-dev.git
VELOX_BRANCH=test
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
+20
to
+22
| VELOX_REPO=https://github.com/rui-mo/velox-dev.git | ||
| VELOX_BRANCH=test | ||
| VELOX_ENHANCED_BRANCH=test |
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
ep/build-velox/src/get-velox.sh:22
- The default Velox repo/branch are changed to a personal fork/"test" branch, which will make builds non-reproducible for other contributors and likely break CI. This script should default to the project-approved Velox repo/branch, and personal testing should be done via the existing --velox_repo/--velox_branch flags.
VELOX_REPO=https://github.com/rui-mo/velox-dev.git
VELOX_BRANCH=test
VELOX_ENHANCED_BRANCH=test
gluten-delta/src/main/scala/org/apache/gluten/execution/DeltaScanTransformer.scala:163
- deltaColumnMappingMode currently handles only NoMapping and NameMapping. Delta tables can also be in IdMapping mode (see GlutenDeltaParquetFileFormat columnMappingMode == IdMapping checks), and in that case the scan split should pass an ID-based mapping mode to Velox; otherwise Velox may fall back to positional/name mapping and read incorrect columns.
private def deltaColumnMappingMode: Option[ColumnMappingMode] = relation.fileFormat match {
case d: DeltaParquetFileFormat =>
d.columnMappingMode match {
case NoMapping => Some(ColumnMappingMode.POSITION)
case NameMapping => Some(ColumnMappingMode.NAME)
case _ => None
}
case _ => None
| import org.apache.spark.sql.catalyst.plans.QueryPlan | ||
| import org.apache.spark.sql.connector.read.streaming.SparkDataStream | ||
| import org.apache.spark.sql.delta.{DeltaParquetFileFormat, NoMapping} | ||
| import org.apache.spark.sql.delta.{DeltaParquetFileFormat, NameMapping, NoMapping} |
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
+20
to
+22
| VELOX_REPO=https://github.com/rui-mo/velox-dev.git | ||
| VELOX_BRANCH=test | ||
| VELOX_ENHANCED_BRANCH=test |
Comment on lines
+156
to
+162
| private def deltaColumnMappingMode: Option[ColumnMappingMode] = relation.fileFormat match { | ||
| case d: DeltaParquetFileFormat => | ||
| d.columnMappingMode match { | ||
| case NoMapping => Some(ColumnMappingMode.NAME) | ||
| case NameMapping => Some(ColumnMappingMode.NAME) | ||
| case _ => None | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Stop passing Parquet/ORC column mapping mode through Velox session configs.
Instead, Gluten resolves the Spark-compatible mapping mode per scan split and sends it through LocalFiles metadata to HiveConnectorSplit. This allows ORC and Parquet scans in the same query to use different matching modes without fallback.
Depends on: facebookincubator/velox#18417.
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Related issue: #6887