felipepessoto commented on PR #13165: URL: https://github.com/apache/gluten/pull/13165#issuecomment-6202098240
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 correctness Please add a Velox insert-only MERGE regression with CDF enabled and: ```text spark.gluten.sql.columnar.backend.velox.expression.eval_simplified=true ``` Please assert the resulting table rows, persisted `numTargetRowsInserted`, and commit-bounded `table_changes` output. Also verify that the counter projection actually runs natively, so Spark fallback cannot hide the issue. The concern is that [`getQueryContextConf`](https://github.com/apache/gluten/blob/0db8d15f237430aaf8072882cc5de3ecdeac31e6/cpp/velox/compute/WholeStageResultIterator.cc#L706-L715) forces expression-stat export on, but still permits simplified evaluation. In the pinned Velox revision, [`Expr::evalSimplifiedImpl`](https://github.com/IBM/velox/blob/6b616f0ae2599ba2aa30bc2d3fcb610a09dc92a5/velox/expression/Expr.cpp#L513-L554) invokes the function without the processed-row accounting done by normal evaluation. The counter can therefore return the correct values while its statistics remain empty, and `ProjectMetricsUpdater` treats a missing slot as zero. For insert-only MERGEs whose CDF is inferred from AddFiles, zero modified-row metrics can suppress real changes. 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 description Could you explicitly state that this targets the `IncrementMetric` patterns generated by Delta DML, rather than general Spark-equivalent behavior for arbitrary user-constructed Catalyst expressions? For example: > This PR targets the IncrementMetric patterns generated by Delta DML. General Spark-equivalent behavior for arbitrary user-constructed Catalyst expression trees is outside this PR's scope. 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. -- 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]
