Repository navigation
Conversation
|
Run Gluten Clickhouse CI on x86 |
5 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
6 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
I support the native-counting approach here. Could you add a targeted regression/guard and clarify the intended scope? I don't think a general-purpose counter redesign needs to be part of this PR. Evaluator-mode correctnessPlease add a Velox insert-only MERGE regression with CDF enabled and: Please assert the resulting table rows, persisted The concern is that This is source-traced, not runtime-reproduced. Please confirm with the regression and ensure counter-bearing plans use a stats-producing evaluator after user overrides, or fall back when that guarantee is unavailable. Scope clarification in the PR descriptionCould you explicitly state that this targets the
The current eligibility check excludes AND/OR ancestors but also accepts counter-bearing lambdas and nullable-ancestor shapes. Those are not placements emitted by the standard Delta DML builders I traced, so I would treat them as support-boundary questions, not evidence that ordinary MERGE fails under default settings. Please clarify whether they are supported and why they cannot occur in supported plans; if they can reach native execution, they need safe fallback or exact accounting. The description should not promise fallback that the code does not enforce. |
felipepessoto
left a comment
felipepessoto
left a comment
There was a problem hiding this comment.
Please see the inline comments about evaluator-mode accounting and the accepted expression scope. These findings are source-traced, not runtime-reproduced.
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Thanks @felipepessoto, all three are fixed: 1b486cc for eval_simplified, 7f5c7a3 for the placement guard. I also added the scope statement to the description. One more thing I noticed while reading the code, not reproduced yet: Velox can push a join filter below the numSourceRows counter, so that metric can undercount in some inner-join MERGEs. It's the same on main and isn't changed here, and table rows and CDF aren't affected. I'll raise it separately. |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
…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.
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.
DeltaProjectExecTransformer stripped every IncrementMetric and the metrics updater credited each one with the operator's output rows. That is only right at the root of a projection. MERGE keeps one counter per clause inside CASE WHEN, so every clause counter was inflated, and a no-op MERGE produced phantom change data feed rows. On Velox, the Delta project transformer now rewrites each IncrementMetric into a call to a native counter function (one slot per distinct metric), and the metric is credited with the rows that function processed. Inside a CASE WHEN branch those are exactly the rows that took the branch, so MERGE's write projection stays native. A projection is counted natively only when every counter is reached from its alias through Alias, IncrementMetric, IF/CASE WHEN values, named_struct, array or NOT; any other parent keeps it on Spark. Filters keep the root-stack strip, and backends without native counting offload a projection only when every counter sits at the root of an alias. A post-transform rule keeps the filter and column drop above a fallen-back projection on Spark. The Velox backend turns on supportNativeIncrementMetric. The Delta UT baseline drops the entries this fixes.
f9719f2 to
6ae58c1
Compare
|
@felipepessoto I split this up to make it easier to review:
Feel free to have another look. |
|
Run Gluten Clickhouse CI on x86 |
…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.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
…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.
|
Run Gluten Clickhouse CI on x86 |
|
The initial estimate when I ran the CI with increment metric offload disabled was fixing 225 tests. The PR removed only 179, do you know if it is because my baseline became stale after Delta 4.4 update? Or do we still have some cases to fix? In my experiment disabling the offload it would also avoid some other unrelated bugs |
Good question @felipepessoto Its not the Delta 4.4 update. Out of your 225:
This PR also drops 22 that weren't in your list (20 ImplicitMergeCasting overflow tests and the 2 no-op MERGE CDC stream tests), which is how it gets to 179. Do you think there is a mistake? |
|
Thanks @malinjawi. BTW I re-run the tests with the offload disabled and the number of now-passing tests is now 173 only, not 225. So, I think we are good. https://github.com/apache/gluten/actions/runs/40755144761?pr=13113 |
|
Run Gluten Clickhouse CI on x86 |
…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.
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.
…d docs Review follow-ups on the Delta side: - The CDF MERGE test now checks that both explodes above the write projection run in its native stage. The old transition check stopped at the projection's own stage, so it could not fail. - New test: with increment_metric blacklisted, MERGE's write projection stays on Spark, and incrementMetricParentsRule keeps Delta's filter above it in the same Spark stage. - IncrementMetricOffload docs: without native counting nothing runs natively; with it, only projections call the slot functions and filters are still credited with their output rows. The name match is there because Delta 2.4 has no IncrementMetric class.
|
Run Gluten Clickhouse CI on x86 |
What changes are proposed in this pull request?
Fixes the inflated Delta DML metrics of #9003 by counting Delta's
IncrementMetricnatively.Stack. The first two commits are #13357 (native counter functions) and #13358 (JVM metrics plumbing). The fourth, fifth, sixth and seventh repeat their review follow-ups, so all of these drop out once those merge. Review the third and last commits, the Delta change: the Delta wiring under
gluten-delta/, turning onsupportNativeIncrementMetricinVeloxBackend, the tests and the Delta UT baseline. It supersedes #13158, whose guard is kept as the fallback for backends without native counting.On main,
DeltaProjectExecTransformerstrips everyIncrementMetricand the metrics updater credits it with the operator's output rows. That is only right at the root of a projection. MERGE keeps one counter per clause insideCASE WHEN, so every clause counter was inflated, and a no-op MERGE produced phantom change-data-feed rows. With this PR:IncrementMetricinto a call to a native counter function, one slot per distinct metric, and the metric is credited with the rows that function processed. Inside aCASE WHENbranch those are exactly the rows that took the branch, as on Spark, so MERGE's write projection stays native.truekeeps every row, so the output row count is exact. Any other filter counter falls back.Scope
This PR targets the
IncrementMetricplacements generated by Delta 3.3 and 4.4 (the Delta release for every Spark 4.x) UPDATE, DELETE and MERGE. General Spark-equivalent counting for arbitrary user-constructed Catalyst trees is out of scope. A projection is counted natively only when every counter is reached from its alias throughAlias,IncrementMetric,IF/CASE WHENvalues (never conditions),named_struct,arrayorNOT(IncrementMetricOffload.countsLikeSpark). Any other parent keeps the projection on Spark:AND/OR, conditions, null-intolerant functions such asa + b,divide,coalesce, lambda bodies, and so on.Known gap, pre-existing on main and unchanged here: Velox can push a hash-join dynamic filter below the projection that counts
numSourceRows, so that metric can undercount for an inner-join MERGE with a broadcast target. Table rows and change data feed are not affected.How was this patch tested?
IncrementMetricOffloadSuite(plugin-free, run on Spark 3.5 and 4.0): slot assignment, the slot limit, the placement allowlist (every Delta DML shape accepted; nullable ancestors, lambdas, conditions andAND/ORrejected) and the non-native guard.DeltaSuiteexact-metric tests: MERGE with delete, update and insert clauses; no-op MERGE with CDF; MERGE with CDF; DELETE/UPDATE with and without DVs. On Velox they assert that MERGE'sCASE WHENprojection runs asDeltaProjectExecTransformer, and with CDF that both explodes above it share its native stage. Another test blacklistsincrement_metricto keep that projection on Spark and checks that the filter Delta puts above it stays on Spark too. The insert-only MERGE test runs withexpression.eval_simplifiedfalse and true and checks the rows,numTargetRowsInserted,table_changes(t, v, v)and, on Delta 3.3, that the counter projection is native (Delta 4.4 counts these inserts with a filter over a literaltrue). It checksnumSourceRowsfrom Spark 4.0 only, because Delta 3.3's data skipping evaluates that counter once more per source file.delta_spark_utgate (Spark 4.1, Delta 4.4.1) shows no regressions; the baseline drops the entries this PR fixes.VeloxDeltaSuite,IncrementMetricOffloadSuite,MetricsUtilSuiteandProjectMetricsUpdaterSuitepass on Spark 3.5 andVeloxDeltaSuiteon Spark 4.1, against a native build of this branch.Was this patch authored or co-authored using generative AI tooling?
Generated-by: IBM Bob
Related issue: #9003