Copilot commented on code in PR #13109:
URL: https://github.com/apache/gluten/pull/13109#discussion_r4146592072
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -1668,11 +1718,27 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
std::vector<common::Subfield>{},
icebergColumn->initialDefault);
} else {
+ std::vector<common::Subfield> requiredSubfields;
+ if (columnType == ColumnType::kRegular &&
!requiredSubfieldsByCol.empty()) {
+ auto it = requiredSubfieldsByCol.find(colNameList[idx]);
+ if (it != requiredSubfieldsByCol.end()) {
+ VLOG(1) << "Map-key pruning: applying " << it->second.size() << "
required subfields to column "
+ << colNameList[idx];
+ requiredSubfields = std::move(it->second);
+ requiredSubfieldsByCol.erase(it);
+ }
+ }
assignments[outName] =
std::make_shared<connector::hive::HiveColumnHandle>(
- colNameList[idx], columnType, veloxTypeList[idx],
veloxTypeList[idx]);
+ colNameList[idx], columnType, veloxTypeList[idx],
veloxTypeList[idx], std::move(requiredSubfields));
}
outNames.emplace_back(outName);
}
+ // A declared column that matched no regular scan column is not applied; the
column is then read
+ // whole, which is correct but not what the plan asked for.
+ for (const auto& [columnName, subfields] : requiredSubfieldsByCol) {
+ LOG(WARNING) << "Map-key pruning: " << subfields.size() << " required
subfields declared for column '" << columnName
+ << "' matched no scan column and were ignored";
Review Comment:
This warning can become noisy in production if there are benign mismatches
(e.g., schema/name normalization differences across components, or feature
interactions where pruning is intentionally ignored). Consider reducing
severity (e.g., `VLOG(1)`/`LOG(INFO)`) or adding rate-limiting/context (scan ID
/ table / query ID) so operators can diagnose without flooding logs.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicScanExecTransformer.scala:
##########
@@ -188,7 +206,12 @@ trait BasicScanExecTransformer extends
LeafTransformSupport with BaseDataSource
val optimization =
BackendsApiManager.getTransformerApiInstance.packPBMessage(
StringValue.newBuilder.setValue(s"isMergeTree=$mergeTreeFlag\n").build)
- val extensionNode = ExtensionBuilder.makeAdvancedExtension(optimization,
null)
+ val enhancement = if (requiredMapSubfields.isEmpty) {
+ null
+ } else {
+
BackendsApiManager.getTransformerApiInstance.packRequiredSubfields(requiredMapSubfields)
+ }
+ val extensionNode = ExtensionBuilder.makeAdvancedExtension(optimization,
enhancement)
Review Comment:
Packing required subfields into `ReadRel.advanced_extension.enhancement`
makes this scan-level channel single-purpose (protobuf `Any` can hold only one
message). If any scan already uses `enhancement` for another read extension
(e.g., Iceberg or future features), this will be mutually exclusive and
silently drop one side. Consider introducing a wrapper enhancement message that
can carry multiple optional sub-messages (or repeated `Any`s), and update both
packing/unpacking to merge extensions rather than overwrite.
##########
backends-velox/src-delta/test/scala/org/apache/gluten/execution/VeloxDeltaSuite.scala:
##########
@@ -49,4 +50,88 @@ class VeloxDeltaSuite extends DeltaSuite {
}
}
}
+
+ Seq("name", "id").foreach {
+ mode =>
+ test(s"map-key pruning is disabled under column mapping mode = $mode") {
+ withTable("delta_cm_map") {
+ spark.sql(s"""
+ |create table delta_cm_map
+ | (id bigint, m map<string, struct<s string, t
bigint>>)
+ |using delta
+ |tblproperties ("delta.columnMapping.mode" = "$mode")
+ |""".stripMargin)
+ spark.sql(
+ "insert into delta_cm_map select id, " +
+ "map('a', named_struct('s', concat('v', cast(id % 7 as string)),
't', id), " +
+ "'b', named_struct('s', 'x', 't', id * 2)) from range(0, 500)")
+ val pruningFlag =
"spark.gluten.sql.columnar.backend.velox.scanMapKeyPruningEnabled"
+ withSQLConf(pruningFlag -> "true", "spark.sql.adaptive.enabled" ->
"false") {
+ runQueryAndCompare(
Review Comment:
The pruning config key is hard-coded here while other tests use
`VeloxConfig.SCAN_MAP_KEY_PRUNING_ENABLED.key`. Using the shared constant would
avoid drift if the key ever changes and keeps config usage consistent across
suites.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/SubfieldPath.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.gluten.execution
+
+/** One step of a [[SubfieldPath]] below its column. */
+sealed trait SubfieldElement
+
+object SubfieldElement {
+
+ /** A struct field, by the schema's name. */
+ case class Field(name: String) extends SubfieldElement
+
+ /** A map lookup by a string key. */
+ case class StringKey(key: String) extends SubfieldElement
+
+ /** A map lookup by an integral key. */
+ case class LongKey(key: Long) extends SubfieldElement
+}
+
+/**
+ * A path from a scan output column into its value, e.g. column `m` with
elements [StringKey("a"),
+ * Field("t")] for `m['a'].t`. Declared on a scan as a required subfield so
the native reader keeps
+ * only the map entries such paths name. Transported structurally (see
+ * RequiredSubfieldsExtension.proto); toString renders the Velox Subfield
syntax for logs and tests.
+ */
+case class SubfieldPath(column: String, elements: Seq[SubfieldElement]) {
+ override def toString: String = {
+ val sb = new StringBuilder(column)
+ elements.foreach {
+ case SubfieldElement.Field(name) => sb.append('.').append(name)
+ case SubfieldElement.StringKey(key) =>
+ sb.append("[\"").append(key.replace("\\", "\\\\").replace("\"",
"\\\"")).append("\"]")
Review Comment:
`toString` is used for logs/tests but currently only escapes backslashes and
quotes. Keys containing other control characters (e.g., newline, tab) will
produce non-portable / multiline output and can make assertions brittle.
Consider a more complete string escaping strategy (e.g., JSON-style escaping
for common control chars) to keep output deterministic and debuggable.
--
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]
