Repository navigation
Conversation
The JVM side of counting Delta's IncrementMetric natively: - IncrementMetricCall, an unevaluable pass-through expression that is sent to the backend as a call to one of its counter functions. - BackendSettingsApi.supportNativeIncrementMetric(), false by default. - The Velox metrics path reads the counters' processed-row counts (expressionStats) from the native operator metrics, merges them across operators, and ProjectMetricsUpdater credits each SQL metric from its counter. Nothing emits IncrementMetricCall yet, so behavior is unchanged.
Contributor
Author
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
IncrementMetricCallunderincrement_metric, butIncrementMetricCall.prettyName… · NewprettyNameis set tofunctionName(e.g.,increment_metric_0), but the Velox expression… · NewprettyNameis set tofunctionName(e.g.,increment_metric_0), but the Velox expression… · NewexpressionStatsis introduced here, but the added test suite covers… · Newaggregated.expressionStatsusingasJavaover a Scala map produced via.map(...).… · New
What changed in this PR
This PR adds support for carrying native “increment metric” processed-row counts from Velox operator metrics into Spark SQL metrics, introducing a pass-through expression (IncrementMetricCall) and wiring expression-level stats through the Velox metrics pipeline.
Changes:
- Introduces
IncrementMetricCalland backend capability flagsupportNativeIncrementMetric(). - Parses, aggregates, and forwards per-expression processed row counts (
expressionStats) from Velox metrics JSON intoOperatorMetrics. - Updates
ProjectMetricsUpdaterand adds a new Velox test suite validating native counter crediting behavior.
| File | Description |
|---|---|
| shims/common/src/main/scala/org/apache/gluten/expression/ExpressionNames.scala | Adds a named expression constant for increment_metric. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/IncrementMetricCall.scala | Adds a new unevaluable pass-through expression used to represent native counter function calls. |
| gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/BackendSettingsApi.scala | Adds a backend capability flag for native increment-metric support. |
| backends-velox/src/test/scala/org/apache/gluten/metrics/ProjectMetricsUpdaterSuite.scala | Adds unit tests covering the updated counter crediting logic. |
| backends-velox/src/main/scala/org/apache/gluten/metrics/ProjectMetricsUpdater.scala | Credits SQL metrics from expressionStats for native counters keyed by function name. |
| backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala | Parses expressionStats from JSON and aggregates it across operators. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala | Wires IncrementMetricCall into the Velox expression signature registry. |
| backends-velox/src/main/java/org/apache/gluten/metrics/OperatorMetrics.java | Adds expressionStats field to carry per-expression processed row counts. |
Review effort: Lite
…ementMetricCall naming Addresses review: - MetricsUtil merges expressionStats into a java.util.HashMap, like the one every OperatorMetrics starts with, instead of wrapping a Scala map with asJava. - MetricsUtilSuite covers reading expressionStats from the native metrics JSON (with entries, missing, empty) and summing it across merged operators. - IncrementMetricCall documents why it is mapped by class under increment_metric while its prettyName stays the slot name: Gluten recognizes Delta's IncrementMetric by the name increment_metric.
malinjawi
added a commit
to malinjawi/incubator-gluten
that referenced
this pull request
Oct 20, 2026
…ementMetricCall naming Addresses review: - MetricsUtil merges expressionStats into a java.util.HashMap, like the one every OperatorMetrics starts with, instead of wrapping a Scala map with asJava. - MetricsUtilSuite covers reading expressionStats from the native metrics JSON (with entries, missing, empty) and summing it across merged operators. - IncrementMetricCall documents why it is mapped by class under increment_metric while its prettyName stays the slot name: Gluten recognizes Delta's IncrementMetric by the name increment_metric. Same change as apache#13358.
Copilot stopped reviewing on behalf of
zhouyuan due to an error
October 20, 2026 19:23
|
Run Gluten Clickhouse CI on x86 |
4 tasks
felipepessoto
approved these changes
Oct 23, 2026
|
Run Gluten Clickhouse CI on x86 |
Review follow-ups: - Only Delta's project transformer emits IncrementMetricCall. A filter counter is offloaded only over a literal true, which keeps every row, so FilterMetricsUpdater keeps crediting its output rows; say so there. - Fix the IncrementMetricCall and OperatorMetrics docs to match. - Read a slot's count with getOrDefault.
|
Run Gluten Clickhouse CI on x86 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Part of #9003. This is the JVM half of #13165, split out to make review easier.
IncrementMetricCall(gluten-substrait): an unevaluable pass-through expression that is sent to the backend as a call to one of its counter functions.BackendSettingsApi.supportNativeIncrementMetric(), false by default. The Velox backend turns it on in [GLUTEN-9003][VL] Count Delta's IncrementMetric natively #13165.expressionStats) from the native operator metrics, merges them across operators, andProjectMetricsUpdatercredits each SQL metric from its counter.FilterMetricsUpdaterneeds no counter branch: Delta's filter counters never run natively. [GLUTEN-9003][VL] Count Delta's IncrementMetric natively #13165 offloads a filter counter only over a literaltrue, which keeps every row, so it is still credited with the filter's output rows.Nothing emits
IncrementMetricCallyet, so behavior doesn't change. The native functions are in #13357.How was this patch tested?
New
ProjectMetricsUpdaterSuite: a counter is credited with the rows its function processed, a counter that never ran with zero, and the existingincrement_metricpath still with the output rows.New
MetricsUtilSuite:expressionStatsis read from the native metrics JSON (with entries, missing, empty) and summed across merged operators.Was this patch authored or co-authored using generative AI tooling?
Generated-by: IBM Bob