Copilot commented on code in PR #12884:
URL: https://github.com/apache/gluten/pull/12884#discussion_r3844995767
##########
cpp/velox/compute/VeloxPlanConverter.cc:
##########
@@ -164,6 +175,15 @@ std::shared_ptr<SplitInfo> parseScanSplitInfo(
metadataColumnMap[metadataColumn.key()] = metadataColumn.value();
}
for (const auto& otherMetadataColumn :
file.other_const_metadata_columns()) {
+ if (otherMetadataColumn.key() == kColumnMappingModeMetadataKey) {
Review Comment:
`kColumnMappingModeMetadataKey` is declared as a `std::string_view`, while
`otherMetadataColumn.key()` is a `std::string`. If this code is compiled under
C++17 (common for Velox ecosystems), `std::string == std::string_view` is not
guaranteed to compile. To avoid a build break, use a compatible type for the
key (e.g., `constexpr const char*` / `constexpr char[]`) or compare via
`otherMetadataColumn.key() == std::string(kColumnMappingModeMetadataKey)` (or
store the key as `std::string`).
##########
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
Review Comment:
Pointing the build to a personal fork and an unpinned branch (`test`) makes
builds non-reproducible and can break CI or downstream consumers unexpectedly.
This should be reverted to an approved upstream (or org-owned) repository and
pinned to a stable branch/commit that matches the stated dependency (Velox PR
18417).
##########
gluten-ut/spark35/src/test/scala/org/apache/spark/sql/hive/execution/GlutenHiveSQLQuerySuite.scala:
##########
@@ -149,6 +149,47 @@ 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 validates the “ORC positional + Parquet name” scenario but does
not explicitly set the Velox Parquet mapping config (e.g., the setting backing
`VeloxConfig.get.parquetUseColumnNames`). To reduce flakiness from suite/global
defaults and to fully cover the new split-level behavior, set the Parquet
mapping conf explicitly within this `withSQLConf` and consider adding a
companion test for “ORC name + Parquet positional” in the same query.
##########
gluten-substrait/src/main/java/org/apache/gluten/substrait/rel/LocalFilesNode.java:
##########
@@ -31,6 +31,8 @@
import java.util.Map;
public class LocalFilesNode implements SplitInfo {
Review Comment:
The metadata key string is duplicated across JVM (`LocalFilesNode`) and C++
(`VeloxPlanConverter`). To avoid silent drift, consider centralizing this key
in a shared location (e.g., a Substrait-related constants definition used to
generate/validate both sides) or, at minimum, add a comment on both sides
pointing to the other definition to keep them in sync.
--
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]
