malinjawi commented on code in PR #13358:
URL: https://github.com/apache/gluten/pull/13358#discussion_r4327971697
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -1404,6 +1404,7 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi with
Logging {
Sig[VeloxBloomFilterAggregate](ExpressionNames.BLOOM_FILTER_AGG),
Sig[MapFilter](ExpressionNames.MAP_FILTER),
Sig[AssertNotNull](ExpressionNames.ASSERT_NOT_NULL),
+ Sig[IncrementMetricCall](ExpressionNames.INCREMENT_METRIC),
Review Comment:
This one's fine as is. Gluten looks expressions up by class, not by
prettyName (`getAndCheckSubstraitName` does
`expressionsMap.get(expr.getClass)`, and this Sig list is merged into that
map), so this entry is exactly what lets `IncrementMetricCall` through.
`getTransformer` then sends the slot name, `increment_metric_N`, to Velox.
#13165 covers it end to end. I added a short note on `IncrementMetricCall` in
5a02cc735 so the naming doesn't trip anyone else up.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/IncrementMetricCall.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 transformers emit it in place of `IncrementMetric`
when the backend
+ * supports native counting. `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.
+ */
+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
Review Comment:
Matching is by class, so prettyName isn't used for the lookup. It actually
needs to stay the slot name: Gluten spots Delta's own `IncrementMetric` by the
name `increment_metric`, so renaming this would make an already rewritten call
look like a Delta counter.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/IncrementMetricCall.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 transformers emit it in place of `IncrementMetric`
when the backend
+ * supports native counting. `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.
+ */
+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)
Review Comment:
Same as above: the lookup is by class, and the slot name is what Velox needs
here.
##########
backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala:
##########
@@ -51,8 +51,20 @@ object MetricsUtil extends Logging {
.map(_.asLong())
.getOrElse(0L)
+ private def expressionStatsFromJson(node: JsonNode): JMap[String,
java.lang.Long] = {
+ val stats = new java.util.HashMap[String, java.lang.Long]()
+ Option(node.get("expressionStats")).foreach {
+ exprStats =>
+ exprStats.fields().asScala.foreach {
+ entry => stats.put(entry.getKey,
java.lang.Long.valueOf(entry.getValue.asLong()))
+ }
+ }
+ stats
+ }
Review Comment:
Good call, added `MetricsUtilSuite` in 5a02cc735. It covers parsing with
entries, missing and empty `expressionStats`, and summing across merged
operators.
--
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]
