Copilot commented on code in PR #12453:
URL: https://github.com/apache/gluten/pull/12453#discussion_r3527861797
##########
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/hive/execution/GlutenHiveSQLQuerySuite.scala:
##########
@@ -149,6 +149,68 @@ class GlutenHiveSQLQuerySuite extends
GlutenHiveSQLQuerySuiteBase {
}
}
+ 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
+
+ withSQLConf("spark.sql.hive.convertMetastoreOrc" -> "false") {
+ withTempDir {
+ dir =>
+ val colStarLoc = s"file:///$dir/test_orc_colstar"
+ val namedLoc = s"file:///$dir/test_orc_named"
Review Comment:
Building `file:///` URIs via string interpolation on a `java.io.File` can
produce invalid/fragile URIs (extra slashes, unescaped characters/spaces, and
Windows path issues). Prefer constructing locations via `dir.toURI.toString`
(and appending subpaths via `new File(dir, \"...\")`) to ensure valid URIs
across environments.
##########
gluten-ut/spark40/src/test/scala/org/apache/spark/sql/hive/execution/GlutenHiveSQLQuerySuite.scala:
##########
@@ -149,6 +149,68 @@ class GlutenHiveSQLQuerySuite extends
GlutenHiveSQLQuerySuiteBase {
}
}
+ 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
Review Comment:
This entire regression test block is duplicated across spark33/34/35/40/41
suites. To reduce drift and simplify future updates, consider extracting the
shared test logic into `GlutenHiveSQLQuerySuiteBase` (e.g., a helper method
that registers the test) and calling it from each Spark-version-specific suite.
##########
gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala:
##########
@@ -586,16 +586,19 @@ object GlutenConfig extends ConfigRegistry {
.foreach { case (k, v) => nativeConfMap.put(k, v) }
// When `orc.force.positional.evolution=true`, vanilla Spark maps ORC
columns by
- // position rather than by name (see OrcUtils.requestedColumnIds). The
Velox ORC reader
- // must do the same, otherwise name-based matching against a mismatched
file schema
- // reads columns back as null/empty. Override the (Velox)
orcUseColumnNames session conf
- // so native reads ORC by position too. Harmless for backends that ignore
this key.
+ // position rather than by name (see OrcUtils.requestedColumnIds). Forward
the flag to
+ // the native (Velox) reader so it maps ORC/DWRF files by position too,
otherwise
+ // name-based matching against a mismatched file schema reads columns back
as null/empty.
+ // The native reader still decides per file (files with all-`_col*`
physical names are
+ // always mapped by position). Harmless for backends that ignore this key.
// String literal is used because gluten-substrait cannot depend on
backends-velox.
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")
}
Review Comment:
The Velox config key is embedded as a raw string literal here. Since
gluten-substrait canโt depend on the Velox module, consider defining a
`private[gluten]` constant in this file/object (e.g.,
`VEL0X_ORC_FORCE_POSITIONAL_EVOLUTION_KEY`) and using it to avoid accidental
typos and make future refactors safer.
##########
docs/velox-configuration.md:
##########
@@ -55,7 +55,6 @@ nav_order: 16
|
spark.gluten.sql.columnar.backend.velox.memoryPoolCapacityTransferAcrossTasks
| ๐ Dynamic | true | Whether to allow memory capacity transfer
between memory pools from different tasks.
|
| spark.gluten.sql.columnar.backend.velox.memoryUseHugePages
| ๐ Dynamic | false | Use explicit huge pages for Velox
memory allocation.
|
| 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
|
Review Comment:
The doc removes `spark.gluten.sql.columnar.backend.velox.orcUseColumnNames`
but doesnโt add any guidance about the new behavior/configuration path. Since
users previously relied on that knob, please document (a) that ORC/DWRF mapping
now follows Spark semantics per file, and (b) that positional evolution is
controlled via Sparkโs `orc.force.positional.evolution` (and/or clarify whether
`spark.gluten.sql.columnar.backend.velox.orcForcePositionalEvolution` is
user-facing and should be listed here).
--
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]
