Copilot commented on code in PR #13358:
URL: https://github.com/apache/gluten/pull/13358#discussion_r4483791060
##########
backends-velox/src/main/scala/org/apache/gluten/metrics/ProjectMetricsUpdater.scala:
##########
@@ -38,6 +40,9 @@ class ProjectMetricsUpdater(
case (name, metric) =>
name match {
case "increment_metric" => metric += operatorMetrics.outputRows
+ case slot if
slot.startsWith(IncrementMetricCall.functionNamePrefix) =>
+ // The counter ran natively: credit the rows Velox evaluated it
on.
+ metric += operatorMetrics.expressionStats.getOrDefault(slot, 0L)
Review Comment:
Using `startsWith(functionNamePrefix)` will treat any metric whose name
begins with `increment_metric_` as a native counter slot, even if it is not
actually a valid slot identifier. This can silently credit incorrect values
(typically 0) for misnamed metrics and makes the behavior sensitive to naming
collisions. Prefer validating that `slot` matches the expected slot format
(e.g., `increment_metric_<number>`) and optionally that the parsed number is
within the supported range; otherwise fall back to the existing
`"increment_metric"` behavior or ignore.
##########
backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala:
##########
@@ -359,6 +371,16 @@ object MetricsUtil extends Logging {
aggregated.bloomFilterTestedRows = bloomFilterTestedRows
aggregated.bloomFilterAcceptedRows = bloomFilterAcceptedRows
aggregated.bloomFilterBypassed = bloomFilterBypassed
+ val expressionStats = new java.util.HashMap[String, java.lang.Long]()
+ operatorMetrics.asScala.foreach {
+ metrics =>
+ metrics.expressionStats.asScala.foreach {
+ case (name, rows) =>
+ val sum =
Option(expressionStats.get(name)).map(_.longValue()).getOrElse(0L) + rows
+ expressionStats.put(name, java.lang.Long.valueOf(sum))
Review Comment:
`mergeMetrics` now assumes `metrics.expressionStats` is always non-null.
Since `expressionStats` is a public mutable field on `OperatorMetrics`, it’s
easy for other construction paths to leave it unset or explicitly set it to
`null`, which would cause an NPE here. A small defensive guard (treat null as
empty) would make this aggregation more robust and reduce coupling to all
`OperatorMetrics` producers always initializing the map.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/IncrementMetricCall.scala:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.expression
+
+import org.apache.spark.sql.catalyst.expressions.{Expression, UnaryExpression,
Unevaluable}
+import org.apache.spark.sql.types.DataType
+
+/**
+ * A native pass-through call that makes the backend count how many rows
reached this point of an
+ * expression tree. Delta's project transformer emits it in place of
`IncrementMetric` when the
+ * backend supports native counting; filters never carry one. `functionName`
is one of the backend's
+ * fixed counter slots and is unique to one SQL metric within its projection:
the backend keys its
+ * per-expression statistics by function name, and the metrics updater credits
the metric from that
+ * entry. Never evaluated on Spark; it only exists in the expression list
handed to the backend.
+ *
+ * Gluten maps it by class (`ExpressionNames.INCREMENT_METRIC`, which is also
the name to put in the
+ * expression blacklist), and `getTransformer` sends `functionName` to the
backend. Its `prettyName`
+ * is the slot name, never `increment_metric`, which is how Gluten recognizes
Delta's own
+ * `IncrementMetric` without depending on Delta.
+ */
+case class IncrementMetricCall(child: Expression, functionName: String)
+ extends UnaryExpression
+ with Unevaluable
+ with Transformable {
+
+ override def dataType: DataType = child.dataType
+
+ override def nullable: Boolean = child.nullable
+
+ override def prettyName: String = functionName
+
+ override protected def withNewChildInternal(newChild: Expression):
IncrementMetricCall =
+ copy(child = newChild)
+
+ override def getTransformer(
+ childrenTransformers: Seq[ExpressionTransformer]): ExpressionTransformer
=
+ GenericExpressionTransformer(functionName, childrenTransformers, this)
+}
+
+object IncrementMetricCall {
+
+ /** Prefix of every counter slot name. */
+ val functionNamePrefix: String = "increment_metric_"
+
+ /**
+ * Number of counter slots the backend registers at startup
(`increment_metric_0` to
+ * `increment_metric_31`). Mirrors `kIncrementMetricFunctionSlots` in the
Velox backend. A
+ * projection with more distinct metrics than this falls back.
+ */
+ val maxCounters: Int = 32
+
Review Comment:
The counter-slot capacity (`maxCounters`) is hard-coded in a shared module
but explicitly “mirrors” a Velox backend constant. This creates a cross-module
coupling that can drift and makes it hard for other backends (or future Velox
changes) to supply a different slot count. Consider moving the slot-count
source behind a backend API (e.g., `BackendSettingsApi` exposes slot capacity)
or making it backend-specific (Velox-only constant) so the JVM side derives
limits from the backend implementation rather than embedding them in
`gluten-substrait`.
--
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]
