Copilot commented on code in PR #12884:
URL: https://github.com/apache/gluten/pull/12884#discussion_r3882153241
##########
cpp/velox/compute/VeloxPlanConverter.cc:
##########
@@ -89,6 +92,14 @@ std::optional<std::string> unpackMetadataValue(const
google::protobuf::Any& valu
return std::nullopt;
}
+std::optional<velox::dwio::common::ColumnMappingMode>
parseColumnMappingMode(const google::protobuf::Any& value) {
+ auto unpacked = unpackMetadataValue(value);
+ if (!unpacked.has_value()) {
+ return std::nullopt;
+ }
+ return
velox::dwio::common::ColumnMappingModeName::tryToColumnMappingMode(*unpacked);
+}
Review Comment:
`LocalFilesNode` encodes `column_mapping_mode` using
`google.protobuf.StringValue` (see updated JSON test data), but
`unpackMetadataValue` never tries to unpack `StringValue`. This will make
`parseColumnMappingMode` return `nullopt` and trip
`VELOX_CHECK(mode.has_value(), ...)` for any split carrying the new metadata.
Add `google::protobuf::StringValue` handling in `unpackMetadataValue` (or
handle it directly in `parseColumnMappingMode`).
##########
gluten-delta/src-delta40/main/scala/org/apache/gluten/delta/DeltaDeletionVectorScanInfo.scala:
##########
@@ -108,6 +147,65 @@ object DeltaDeletionVectorScanInfo {
PartitionFileScanInfo(normalizedMetadata, dvInfo)
}
+ private def extract(
+ file: PartitionedFile,
+ hadoopConf: Configuration,
+ tablePath: Path,
+ addFile: AddFile): PartitionFileScanInfo = {
+ val metadata = otherMetadataColumns(file)
+ val normalizedMetadata = metadata -- Seq(RowIndexFilterIdEncoded,
RowIndexFilterTypeKey)
+ val dvInfo = Option(addFile.deletionVector) match {
+ case Some(descriptor) =>
+ DeletionVectorInfo(
+ true,
+ IF_CONTAINED,
+ descriptor.cardinality,
+ serializePayload(hadoopConf, tablePath, descriptor))
+ case None =>
+ DeletionVectorInfo(false, KEEP_ALL, 0L, Array.emptyByteArray)
+ }
+ PartitionFileScanInfo(normalizedMetadata, dvInfo)
+ }
+
+ private def findAddFile(
+ file: PartitionedFile,
+ tablePath: Path,
+ addFiles: Seq[AddFile]): AddFile = {
+ val partitionedFilePath = new Path(file.filePath.toString)
+ addFiles
+ .find {
+ addFile =>
+ val addFilePath =
DeltaFileOperations.absolutePath(tablePath.toString, addFile.path)
+ samePath(partitionedFilePath, addFilePath)
+ }
+ .getOrElse {
+ throw new IllegalStateException(
+ s"Unable to find Delta AddFile metadata for split ${file.filePath}")
+ }
+ }
Review Comment:
`normalizeFromAddFiles` calls `findAddFile` for each `PartitionedFile`, and
`findAddFile` does a linear scan over `addFiles`. For large scans this can
become O(N*M). Consider pre-indexing `addFiles` once per call (e.g., Map keyed
by a normalized absolute-path representation compatible with `samePath`) and
then doing O(1) lookups per `PartitionedFile`.
##########
backends-velox/src-iceberg/test/scala/org/apache/gluten/execution/VeloxIcebergSuite.scala:
##########
@@ -27,6 +27,32 @@ import org.apache.iceberg.spark.source.SparkTable
import org.apache.iceberg.types.{Type, Types}
class VeloxIcebergSuite extends IcebergSuite {
+ test("iceberg parquet split uses name mapping for projected columns") {
+ withTable("iceberg_parquet_name_mapping") {
+ withSQLConf(VeloxConfig.PARQUET_USE_COLUMN_NAMES.key -> "false") {
+ spark.sql("""
+ |CREATE TABLE iceberg_parquet_name_mapping (
+ | id BIGINT,
+ | amount DECIMAL(12, 2),
+ | note STRING
+ |)
+ |USING iceberg
+ |TBLPROPERTIES ('write.format.default' = 'parquet')
+ |""".stripMargin)
+ spark.sql("""
+ |INSERT INTO iceberg_parquet_name_mapping
+ |VALUES (CAST(1 AS BIGINT), CAST(10.50 AS DECIMAL(12, 2)),
'a')
+ |""".stripMargin)
+
+ runQueryAndCompare("SELECT amount FROM iceberg_parquet_name_mapping") {
+ df =>
+ checkAnswer(df, Seq(Row(BigDecimal("10.50"))))
+ checkGlutenPlan[IcebergScanTransformer](df)
+ }
+ }
+ }
+ }
Review Comment:
This test name claims the split 'uses name mapping', but the assertions only
validate the result and that an `IcebergScanTransformer` exists. Since
projecting a single column can succeed under both position- and name-based
matching, this may not catch regressions in the column-mapping-mode
propagation. Consider making the test sensitive to the mapping mode (e.g.,
enforce a schema evolution/reorder scenario that would fail under position
mapping, or assert the emitted split metadata contains the expected
`__gluten.column_mapping_mode`).
##########
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:
Adding both `PARQUET_FIELD_ID` and `FIELD_ID` introduces
ambiguous/overlapping API surface for what appears to be the same concept. If
both are required for compatibility, consider documenting which engines/files
require which value; otherwise, prefer a single canonical enum value that
matches the native Velox `ColumnMappingModeName` strings to avoid mismatches.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -17,9 +17,9 @@
set -exu
CURRENT_DIR=$(cd "$(dirname "$BASH_SOURCE")"; pwd)
-VELOX_REPO=https://github.com/IBM/velox.git
-VELOX_BRANCH=dft-2026_08_21
-VELOX_ENHANCED_BRANCH=ibm-2026_08_21
+VELOX_REPO=https://github.com/rui-mo/velox-dev.git
+VELOX_BRANCH=test
+VELOX_ENHANCED_BRANCH=test
Review Comment:
The build script now pulls Velox from a personal fork and a non-versioned
`test` branch, which breaks reproducibility and is risky for CI/release builds.
Please revert to an official/upstream repo+pin (commit SHA or release branch),
or make these overrideable via environment variables while keeping a stable,
upstream default.
##########
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/hive/execution/GlutenHiveSQLQuerySuite.scala:
##########
@@ -149,6 +149,48 @@ class GlutenHiveSQLQuerySuite extends
GlutenHiveSQLQuerySuiteBase {
}
}
+ testGluten("ORC positional and Parquet name mapping can coexist in one Velox
query") {
+ val hiveClient: HiveClient =
+
spark.sharedState.externalCatalog.unwrapped.asInstanceOf[HiveExternalCatalog].client
+
+ withSQLConf(
+ "spark.sql.hive.convertMetastoreOrc" -> "false",
+ "spark.sql.hive.convertMetastoreParquet" -> "false",
+ "spark.hadoop.orc.force.positional.evolution" -> "true"
Review Comment:
This test asserts 'ORC positional and Parquet name mapping can coexist', but
it only explicitly forces ORC positional evolution and relies on Parquet's
mapping default. To avoid future default/config drift making the test flaky,
set the Parquet split mapping explicitly (e.g., the Velox Parquet mapping conf
that drives name-vs-position on the Spark side) within `withSQLConf`.
--
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]
