[GLUTEN-12436][VL] Map ORC files by position per-file to match Spark's OrcUtils, and remove the orcUseColumnNames config#12453
Open
beliefer wants to merge 3 commits into
Open
[GLUTEN-12436][VL] Map ORC files by position per-file to match Spark's OrcUtils, and remove the orcUseColumnNames config#12453beliefer wants to merge 3 commits into
beliefer wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Aligns Gluten’s ORC/DWRF column mapping behavior with vanilla Spark by enabling per-file mapping decisions (name vs. position) and removing the redundant global orcUseColumnNames flag.
Changes:
- Added regression tests ensuring
_col*ORC files fall back to positional mapping without settingorc.force.positional.evolution, while real-named ORC files in the same session still map by name. - Replaced the removed Velox ORC “use column names” session behavior with forwarding Spark’s
orc.force.positional.evolutioninto a new native Velox flag. - Updated Velox iterator/schema attachment and cleaned up configs/docs/tests referencing
orcUseColumnNames.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-ut/spark33..41/.../GlutenHiveSQLQuerySuite.scala | Adds a regression test for mixed per-file ORC column mapping modes within one session/query. |
| gluten-substrait/.../GlutenConfig.scala | Forwards Spark positional evolution flag to native via a new Velox config key. |
| cpp/velox/utils/ConfigExtractor.cc | Forces ORC name-mapping default and forwards new positional evolution session property to Velox. |
| cpp/velox/config/VeloxConfig.h | Replaces old ORC config constant with the new positional evolution config key. |
| backends-velox/.../VeloxIteratorApi.scala | Always attaches schema for ORC/DWRF to enable per-file mapping in native reader. |
| backends-velox/.../VeloxConfig.scala | Removes orcUseColumnNames config and accessor. |
| backends-velox/.../VeloxScanSuite.scala | Updates ORC evolution test to use Spark’s orc.force.positional.evolution. |
| backends-velox/.../FallbackSuite.scala | Removes ORC config dimension now that orcUseColumnNames is gone. |
| backends-velox/.../VeloxBackend.scala | Adjusts schema validation gating after ORC orcUseColumnNames removal. |
| docs/velox-configuration.md | Removes orcUseColumnNames from documented configuration list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+167
to
+168
| val colStarLoc = s"file:///$dir/test_orc_colstar" | ||
| val namedLoc = s"file:///$dir/test_orc_named" |
Comment on lines
+152
to
+162
| testGluten( | ||
| "GLUTEN: Hive ORC files with _col* names read by position without positional flag") { | ||
| // Regression for the case where two ORC tables must use OPPOSITE column | ||
| // mapping modes in the same query: one with real column names (by name) and | ||
| // one written by old Hive with placeholder _col* names (by position). The | ||
| // native reader must decide the mode per file (matching vanilla Spark's | ||
| // OrcUtils.requestedColumnIds), so a _col* file reads correctly even though | ||
| // orc.force.positional.evolution is NOT set (ORC is read by name by | ||
| // default). Without the fix the _col* columns would read back as NULL. | ||
| val hiveClient: HiveClient = | ||
| spark.sharedState.externalCatalog.unwrapped.asInstanceOf[HiveExternalCatalog].client |
Comment on lines
57
to
58
| | spark.gluten.sql.columnar.backend.velox.orc.scan.enabled | 🔄 Dynamic | true | Enable velox orc scan. If disabled, vanilla spark orc scan will be used. | | ||
| | spark.gluten.sql.columnar.backend.velox.orcUseColumnNames | 🔄 Dynamic | true | Maps table field names to file field names using names, not indices for ORC files. | | ||
| | spark.gluten.sql.columnar.backend.velox.parquet.dictionaryPageSizeBytes | 🔄 Dynamic | 2MB | The maximum size in bytes for a Parquet dictionary page | |
Comment on lines
595
to
602
| if ( | ||
| backendName == "velox" && | ||
| conf.getOrElse(SPARK_ORC_FORCE_POSITIONAL_EVOLUTION, "false").toBoolean | ||
| ) { | ||
| nativeConfMap.put("spark.gluten.sql.columnar.backend.velox.orcUseColumnNames", "false") | ||
| nativeConfMap.put( | ||
| "spark.gluten.sql.columnar.backend.velox.orcForcePositionalEvolution", | ||
| "true") | ||
| } |
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
235
to
238
| def validateDataSchema(): Option[String] = { | ||
| if (VeloxConfig.get.parquetUseColumnNames && VeloxConfig.get.orcUseColumnNames) { | ||
| if (VeloxConfig.get.parquetUseColumnNames) { | ||
| return None | ||
| } |
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
235
to
238
| def validateDataSchema(): Option[String] = { | ||
| if (VeloxConfig.get.parquetUseColumnNames && VeloxConfig.get.orcUseColumnNames) { | ||
| if (VeloxConfig.get.parquetUseColumnNames) { | ||
| return None | ||
| } |
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
+236
to
238
| if (VeloxConfig.get.parquetUseColumnNames) { | ||
| return None | ||
| } |
Comment on lines
316
to
338
| format => | ||
| Seq("true", "false").foreach { | ||
| parquetUseColumnNames => | ||
| Seq("true", "false").foreach { | ||
| orcUseColumnNames => | ||
| withSQLConf( | ||
| VeloxConfig.PARQUET_USE_COLUMN_NAMES.key -> parquetUseColumnNames, | ||
| VeloxConfig.ORC_USE_COLUMN_NAMES.key -> orcUseColumnNames | ||
| ) { | ||
| withTable("test") { | ||
| spark | ||
| .range(100) | ||
| .selectExpr("to_timestamp_ntz(from_unixtime(id % 3)) as c1", "id as c2") | ||
| .write | ||
| .format(format) | ||
| .saveAsTable("test") | ||
|
|
||
| runQueryAndCompare(query) { | ||
| df => | ||
| val plan = df.queryExecution.executedPlan | ||
| assert(collect(plan) { case g: GlutenPlan => g }.nonEmpty) | ||
| } | ||
| } | ||
| withSQLConf( | ||
| VeloxConfig.PARQUET_USE_COLUMN_NAMES.key -> parquetUseColumnNames | ||
| ) { | ||
| withTable("test") { | ||
| spark | ||
| .range(100) | ||
| .selectExpr("to_timestamp_ntz(from_unixtime(id % 3)) as c1", "id as c2") | ||
| .write | ||
| .format(format) | ||
| .saveAsTable("test") | ||
|
|
||
| runQueryAndCompare(query) { | ||
| df => | ||
| val plan = df.queryExecution.executedPlan | ||
| assert(collect(plan) { case g: GlutenPlan => g }.nonEmpty) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
…OrcUtils, and remove the orcUseColumnNames config
|
Run Gluten Clickhouse CI on x86 |
Comment on lines
235
to
238
| def validateDataSchema(): Option[String] = { | ||
| if (VeloxConfig.get.parquetUseColumnNames && VeloxConfig.get.orcUseColumnNames) { | ||
| if (VeloxConfig.get.parquetUseColumnNames) { | ||
| return None | ||
| } |
Comment on lines
315
to
+321
| Seq("parquet", "orc").foreach { | ||
| format => | ||
| Seq("true", "false").foreach { | ||
| parquetUseColumnNames => | ||
| Seq("true", "false").foreach { | ||
| orcUseColumnNames => | ||
| withSQLConf( | ||
| VeloxConfig.PARQUET_USE_COLUMN_NAMES.key -> parquetUseColumnNames, | ||
| VeloxConfig.ORC_USE_COLUMN_NAMES.key -> orcUseColumnNames | ||
| ) { | ||
| withTable("test") { | ||
| spark | ||
| .range(100) | ||
| .selectExpr("to_timestamp_ntz(from_unixtime(id % 3)) as c1", "id as c2") | ||
| .write | ||
| .format(format) | ||
| .saveAsTable("test") | ||
|
|
||
| runQueryAndCompare(query) { | ||
| df => | ||
| val plan = df.queryExecution.executedPlan | ||
| assert(collect(plan) { case g: GlutenPlan => g }.nonEmpty) | ||
| } | ||
| } | ||
| withSQLConf( | ||
| VeloxConfig.PARQUET_USE_COLUMN_NAMES.key -> parquetUseColumnNames | ||
| ) { |
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?
Fixies #12436. This PR is related to facebookincubator/velox#18027
Gluten currently chooses ORC/DWRF column mapping (by name vs. by position) globally for a whole session, driven by
spark.gluten.sql.columnar.backend.velox.orcUseColumnNames(defaulttrue, map by name) combined with an override fororc.force.positional.evolution.Vanilla Spark does not work that way.
OrcUtils.requestedColumnIdsdecides the mapping mode per file:Because Gluten only had a single global switch, it could not match Spark when a single query touches ORC tables that require opposite mapping modes — for example a join between:
_col0, _col1, ...(must be read by position).With the global switch, one value is always wrong for one of the tables:
orcUseColumnNames=truereads the_col*table back as NULL (name lookup fails), while settingorc.force.positional.evolution=trueglobally forces the real-named table to be read by position and mis-binds its columns.This PR aligns Gluten with Spark's per-file behavior (backed by a companion Velox change) and, as requested, removes the now-redundant
orcUseColumnNamesconfig and its accessor.How the per-file decision works now
The native (Velox) reader makes the decision per ORC/DWRF file:
_col*placeholder names is mapped by position, regardless of any flag;orc.force.positional.evolution=trueis forwarded to native;For this to work, Gluten must always hand the table (data) schema to the native reader for ORC/DWRF so it has a target to remap file columns to.
Notes
This is the Gluten side of a two-part change. The companion Velox change (see: facebookincubator/velox#18027) adds per-file positional mapping in the DWRF reader: it maps a file by position when the physical schema is all
_col*placeholder names or when theorc.force-positional-evolutionsession property is set. This Gluten PR depends on that Velox change being present in the Velox build.How was this patch tested?
gluten-ut(spark33/34/35/40/41)GlutenHiveSQLQuerySuite— new regression test: two ORC tables over the same_col*files (placeholder names) plus a real-named table, all read in one session without setting the positional flag. Asserts the_col*table reads correctly (positional fallback), the real-named table reads correctly by name (opposite mode), and a join of the two returns a non-empty result (the original failure folded the join to an emptyLocalTableScan).VeloxScanSuite— the former "ORC index based schema evolution" test (which relied onorcUseColumnNames=falseto read real-named files by index, a non-Spark behavior) is rewritten as "ORC positional schema evolution" usingorc.force.positional.evolution=true, which produces the same positional mapping via the Spark-compatible path. Expected results are unchanged.FallbackSuite— the "fallback with index based schema evolution" test drops theorcUseColumnNamesdimension (the parquet dimension is retained).Was this patch authored or co-authored using generative AI tooling?
co-authored Claude code.