Repository navigation
Conversation
Contributor
Author
…etric Delta's IncrementMetric counts how many rows an expression is evaluated on. Velox already keeps that count per expression (ExprStats::numProcessedRows), so a counter can be a pass-through function whose name is the key the JVM reads the count from. - Register 32 fixed counter slots, increment_metric_0 to increment_metric_31: non-deterministic, so Velox never constant-folds or dictionary-peels them, and without default null behavior, so null inputs still count. - For a plan that calls a counter, force operator_track_expression_stats on and expression.eval_simplified off after the user's overrides (simplified evaluation applies functions without counting them), and export the counters' processed-row counts in the operator metrics. Nothing calls the counters yet, so plans and metrics are unchanged. The Delta transformers start emitting them in a follow-up.
malinjawi
force-pushed
the
feat/velox-increment-metric-counters
branch
from
October 19, 2026 12:05
84f1bb3 to
438aca7
Compare
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
4 open issues
This addsexpressionStatsto the operator metrics JSON unconditionally (often as an empty… · NewisIncrementMetricFunctioncurrently treats any name with theincrement_metric_prefix as a… · New Plan detection only inspectsProjectNodeandFilterNode. If counters ever appear in other plan… · NewtrackExpressionStats_reads like a direct reflection of the query config, but the comment… · New
What changed in this PR
Adds Velox-native support for Delta’s IncrementMetric by introducing fixed-slot counter functions and wiring their per-expression processed-row counts into operator metrics, while forcing the necessary query config knobs when counters are present in the plan.
Changes:
- Added
increment_metric_<slot>(0–31) passthrough vector functions and registration. - Added plan detection + query-config forcing to ensure expression stats are tracked and simplified eval is disabled when counters are used.
- Exported counter processed-row counts into operator metrics JSON under
expressionStats; added unit tests.
| File | Description |
|---|---|
| cpp/velox/operators/functions/IncrementMetricFunction.h | Declares fixed-slot counter APIs, plan detection, and config overrides. |
| cpp/velox/operators/functions/IncrementMetricFunction.cc | Implements counter function registration, counter-name detection, plan traversal, and config override helper. |
| cpp/velox/operators/functions/RegistrationAllFunctions.cc | Registers the new counter functions during startup registration. |
| cpp/velox/compute/WholeStageResultIterator.h | Adds a flag to track whether the plan uses counters (to force configs). |
| cpp/velox/compute/WholeStageResultIterator.cc | Forces configs when counters are detected; exports counter stats into operator metrics JSON. |
| cpp/velox/tests/IncrementMetricFunctionTest.cc | Adds tests for passthrough behavior, counting semantics, plan detection, and config-forcing. |
| cpp/velox/tests/CMakeLists.txt | Adds the new test file to the existing test target. |
| cpp/velox/CMakeLists.txt | Adds the new implementation file to the build. |
Review effort: Lite
…expressionStats Review follow-ups: - isIncrementMetricFunction accepts only increment_metric_0 to increment_metric_31, not any name with the prefix. - An operator's metrics JSON gets an expressionStats key only when it has counter stats; the JVM already reads a missing key as empty. - Rename trackExpressionStats_ to usesIncrementMetricFunctions_, which is what it stores. - Say why only project and filter nodes are inspected: they are the only ones Gluten puts counters in.
Copilot stopped reviewing on behalf of
zhouyuan due to an error
October 23, 2026 18:15
4 tasks
felipepessoto
approved these changes
Oct 23, 2026
…s counter shapes Review follow-ups: - Counters only appear in projections: the JVM emits them from Delta's project transformer alone, and its filter transformer strips its counters and credits the filter's output rows. The comments said "project and filter"; usesIncrementMetricFunctions now looks at projections only. - checkIncrementMetricFunctionsInProjects fails the plan conversion when the Substrait plan declares a counter that no projection calls. FilterProject only reports the counts of its projection, so a counter anywhere else would read zero. - Test the shapes Delta emits: counters over literals, per-branch counts with the IF result checked, a dictionary input that must not be peeled, and the new check. - Rename the per-operator JSON object that shadowed the node's stats.
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 native half of #13165, split out to make review easier.
Delta's
IncrementMetriccounts how many rows an expression is evaluated on, and Velox already tracks that per expression (ExprStats::numProcessedRows). This adds the native side of counting it there:increment_metric_0toincrement_metric_31. Each one is a pass-through, marked non-deterministic so Velox never constant-folds or dictionary-peels it, and without default null behavior so null inputs still count.WholeStageResultIteratorforcesoperator_track_expression_stats=trueandexpression.eval_simplified=falseafter the user's overrides (simplified evaluation doesn't count rows), and exports the counters' processed-row counts with the operator metrics.Nothing calls these functions yet, so plans, metric values and the metrics JSON don't change: an operator's metrics get an
expressionStatsobject only when its plan calls a counter. #13358 reads the counts on the JVM side, and #13165 makes the Delta transformers emit the calls.How was this patch tested?
IncrementMetricFunctionTest: pass-through including nulls; the shapes Delta emits (counters over literals, per-branch counts insideifwith the result checked); a dictionary input, which must not be peeled; detecting counters in a plan; rejecting a counter outside a projection; and that the forced configs keep counting when a user setseval_simplified=trueandoperator_track_expression_stats=false.Was this patch authored or co-authored using generative AI tooling?
Generated-by: IBM Bob